dialects: (x86) PR5_1 - single operand instructions - one source

[KGrykiel]

Added support for single operand instructions and the following specific cases: idiv, pop, push and not. I added some tests for the verification of the single operand instructions given that the one operand can be a destination, a source or both.

idiv is a bit of a special case since it technically has 2 destination registers: RAX for quotient and RDX for remained, but they're not specified by the programmer but rather built-in. I'd be happy to discuss what the best approach to this is and if it should create SSA values.

[codecov]

Codecov Report

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

Project coverage is 89.72%. Comparing base (0a69f90) to head (493cf0a).
Report is 38 commits behind head on main.

Files	Patch %	Lines
xdsl/dialects/x86/ops.py	75.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2398      +/-   ##
==========================================
+ Coverage   89.64%   89.72%   +0.08%     
==========================================
  Files         346      350       +4     
  Lines       41175    42567    +1392     
  Branches     6120     6331     +211     
==========================================
+ Hits        36911    38194    +1283     
- Misses       3366     3437      +71     
- Partials      898      936      +38     

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

[superlopuh]

I would recommend sticking to filecheck tests for everything that can be checked with filecheck, which I think is the case for these. For verification errors search for --verify-diagnostics in the project to find some examples. Having the expected error string is quite useful.

[KGrykiel]

Oh, I was not aware of the verify-diagnostics, I’ll change that 👍

[superlopuh]

why optional? I'd expect these to be required

[KGrykiel]

It’s to support different types of operations eg. pop has only a destination while push has only a source.

[superlopuh]

It sounds like they should be different operations, one with only a source, and one with only a destination. Otherwise a user may construct a pop with only a source.

[KGrykiel]

I made the verification methods to avoid that, but can split them into two methods as well if that’s better

[superlopuh]

Yes please, verification is good, but declarative structures are better, that way we get more warnings statically rather than at runtime.

[KGrykiel]

What to do if the operand is both a source and a destination like in the 'not' instruction? a seperate class for this case as well?

Sorry for no progress by the way, i'll get on it after I'm done with the dissertation writing,

[superlopuh]

Then I'd say we want an instruction that has a source and target register, and constrains the type of the operand and result to be exactly the same, with a ConstraintVar, something like this:

    T = Annotated[RegisterType, ConstraintVar("T")]
    operand: Operand = operand_def(T)
    result: OpResult = result_def(T)

[ShaolunWang]

I feel like for things like assert push_op.assembly_line() == " push rdx" might be hard to update in the future since you are hardcoding the white spaces. Something like e.g. assert push_op.assembly_line().lstrip() == "push rdx" might be easier to update. Otherwise LGTM 👍

[superlopuh]

I disagree, I think that testing for string equality is better in general, and in this case, as we add the whitespace to make the assembly more uniform/readable, and would like to at least be notified when the format changes.

[webmiche]

IMO, in a perfect world, I would love it if you could encode the fact that rax and rdx are the needed registers in a declarative way. I imagine something like:

class IdivOp:

  divisend = operand_def(x86RegisterType("rdx"))
  divisor = operand_def(x86RegisterType("rax"))
  quotient = ...
  remainder = ...

I am honestly not sure if this will be possible in the current xDSL infrastructure and with the way you have set up the x86Registertype. But it would still be cool to do somehow 😅

IMO, it might make sense to remove Idiv from this PR and submit one for this specifically, then we could have this discussion there and not stall this one :)

[superlopuh]

Yeah TBH we could probably open separate PRs for a few of these ops to be able to discuss these things separately. I understand that this was the original intent of this PR, but it feels like "single operand instruction" is not quite the abstraction we're looking for

[KGrykiel]

As per the suggestion I decided to split up the PR into one for each - 1 source, 1 destination and 1 source/destination case. This is going to be for 1 source. I will set up the other PRs shortly.