dialects: (x86) PR1 - Registers

[KGrykiel]

This is the first of several pull requests that are intended to upstream the work from the KGrykiel/x86dialect branch. In this one I set up registers that are going to be used by the x86 dialect

[codecov]

Codecov Report

Attention: Patch coverage is 91.89189% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 89.56%. Comparing base (d51bcd0) to head (df1730d).
Report is 7 commits behind head on main.

Files	Patch %	Lines
xdsl/dialects/x86/register.py	83.01%	6 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2333      +/-   ##
==========================================
+ Coverage   89.53%   89.56%   +0.02%     
==========================================
  Files         335      341       +6     
  Lines       40327    40530     +203     
  Branches     6028     6037       +9     
==========================================
+ Hits        36108    36301     +193     
- Misses       3347     3353       +6     
- Partials      872      876       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[AntonLydike]

LGTM! I assume there are more register types coming later on?

[KGrykiel]

yep, AVX registers are coming as well

[webmiche]

Could you add some tests for this? At least instantiate some of the registers in a filecheck test or something.

If you don't want to, feel free to move this to the experimental folder.

[webmiche]

(Sidenote: our naming standard says that the dialect name should be in brackets 😉 )

[KGrykiel]

Could you add some tests for this? At least instantiate some of the registers in a filecheck test or something.

If you don't want to, feel free to move this to the experimental folder.

I have some tests that use the register class, they'll be added in later PRs 👍

[superlopuh]

Can you please add the tests in this PR so that we can review them at the same time as the code they're testing?

[KGrykiel]

the tests make use of the registers, but are for specific instructions so I can't add them until i also add the instructions. Unless you'd like me to write a test just for initialising a register?

[webmiche]

Yes, please, I think that would make sense 👍

[KGrykiel]

let me know if this is what you had in mind

[KGrykiel]

formatting fails because of a star import in init, but not sure how to avoid it.

[superlopuh]

Looking great! Can you please take a look at the codecov, and cover the remaining lines? We don't always do this, but it feels like for such a foundational type it would be good to cover all the bases. For example, we don't have any tests for parsing and printing, but it would be good to add a filecheck test for these. Something like this:

%a = "test.op"() -> !x86.reg<rax>

[KGrykiel]

I have a filecheck coming up in next PRs that has just that by using the newly implemented instructions, so that base should be covered 👍

[webmiche]

A couple more comments from my side.

[webmiche]

Why do you have both x86RegisterType or GeneralRegisterType and both have the same description?

[KGrykiel]

x86RegisterType is the parent class for multiple types of register collections. Other than GeneralRegisterType, AVXRegisterType will also inherit from it. A similar system has been implemented in the riscv dialect

[webmiche]

Could you somehow represent this in the documentation? One might be an abstract registertype, the other one a specifically scalar registertype

[superlopuh]

Ah, that's a good idea

Suggested change

	class X86RegisterType(RegisterType):
	import abc
	class X86RegisterType(RegisterType, abc.ABC):

Having it marked as explicitly abstract and not possible to instantiate would be useful in itself

[superlopuh]

I appreciate that they'll be further covered in downstream PRs, but it would still be worth adding the tests for the code being added in the same PR, so we can review the two together. I don't know what the tests are that are still upcoming, so I cannot say now whether they will cover the functionality introduced here in the future.

[KGrykiel]

Ok, will add a filecheck now as well then 🫡

[superlopuh]

Looks perfect, thank you!

[webmiche]

Nice, LGTM now. I agree that marking the X86RegisterType as an ABC would make a lot of sense, but if you don't think so, that's fine as well.