dialects: (x86) PR5_2 - single operand instructions - one destination

[KGrykiel]

next part of PR #2398. added the case where the single operand is a destination + the pop operation

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.81%. Comparing base (eb83cde) to head (42766fe).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2455      +/-   ##
==========================================
+ Coverage   89.77%   89.81%   +0.03%     
==========================================
  Files         350      350              
  Lines       42747    42878     +131     
  Branches     6353     6370      +17     
==========================================
+ Hits        38376    38510     +134     
+ Misses       3434     3423      -11     
- Partials      937      945       +8     

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

[superlopuh]

This instruction doesn't have any operands, am I missing something?

[KGrykiel]

This is by design although worth discussing. The pop function moves data from the top of the stack to a destination register. Whatever is inside the register is irrelevant, so I thought it best to only designate it as a result rather than an operand.

[superlopuh]

That's exactly the right design, IMO, I don't see why inherit from that class

[KGrykiel]

oh, right, in that sense. I used the single/double/tripleOperandInstruction classes as a way to differentiate the instructions over how they look in assembly. In the sense how many arguments, whatever that may be, an assembly instruction takes. I suppose pop creates this kind of gray area where the assembly instruction takes an argument (i.e. "pop rax") but it's not really an operand.

In hindsight this might be a little confusing, but I'm not sure how best to handle the software engineering aspect of class hierarchy in this case. Now that I've implemented more instructions since first devising the plan, I couldn't find anything meaningful to put in the single/double/tripleOperandInstruction parent classes so perhaps it's best to get rid of them all together and have everything inherit from x86Instruction directly?

I'm fine with whatever you think is best tbh

[superlopuh]

I think we might as well get rid of them, we have another bunch of abstract class helpers for the inits already, so we might as well drop the marker classes.

[KGrykiel]

Ok, I opened a new issue for removing the parent classes (#2466), I'll do it later because now it would mess up merging for the other PR I have open.

Going forward with this, would you rather I categorize the instructions by the number of arguments the assembly instruction takes or by the actual number of operands including the intrinsic ones? In this example the 'push' instruction for instance could be an ROperation (because in the assembly it would look like 'push rdx') or an RR Operation (because it technically affects 2 registers). What are your thoughts?

[superlopuh]

I wouldn't think of it as a big taxaonomy, the only reason we added it to riscv was to have less duplication of code for the initializers, I'd just group them by the init signature and not overthink it.

[tarinduj]

I think our current parent class naming convention (eg: add rax, rbx: RROperatio) only makes sense when one of the operands is also the result. IMO renaming the parent classes to reflect other cases as well would make more sense. Something like [Result]-[Operands]-Operation (eg: add rax, rbx: R-RR-Operation). So, push would technically be M-R-Operation (because it's dereferencing rsp)?

I think it would be helpful to de-sugar the assembly and keep the information explicitly in the SSA.

[KGrykiel]

That's also an option, i can change that in a fell swoop with the removal of the parent classes 👍

[tarinduj]

If not, we can think of push and pop as a mov and do the same thing we do there. Whichever makes more sense.

[KGrykiel]

@tarinduj @superlopuh let me know if this is what you had in mind regarding %rsp. If so I imagine I'll have to give the same treatment to the 'push' instruction in a different PR.