Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dialects (arm): add mul instruction with 2 source registers #3515

Merged
merged 2 commits into from
Nov 26, 2024

Conversation

emmau678
Copy link
Contributor

Add mul instruction (https://developer.arm.com/documentation/ddi0597/2024-06/Base-Instructions/MUL--MULS--Multiply-?lang=en).

The mul instruction supports either 2 or 3 operands (in the case of 2, the destination is also treated as the 2nd source). This PR only handles the case where 3 operands are provided, 2 source registers and 1 destination.

@emmau678 emmau678 added the dialects Changes on the dialects label Nov 25, 2024
@emmau678 emmau678 self-assigned this Nov 25, 2024
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.33%. Comparing base (7ebcb70) to head (55ef701).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3515      +/-   ##
==========================================
- Coverage   90.33%   90.33%   -0.01%     
==========================================
  Files         464      464              
  Lines       58157    58165       +8     
  Branches     5557     5559       +2     
==========================================
+ Hits        52536    52542       +6     
- Misses       4190     4192       +2     
  Partials     1431     1431              

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

Copy link
Collaborator

@alexarice alexarice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

What's the plan for the 2 operand case? I believe the work I'm doing to fix the bufferization operation would allow you to have an optional result here, though that might mess with your naming scheme.

@emmau678
Copy link
Contributor Author

Looks good.

What's the plan for the 2 operand case? I believe the work I'm doing to fix the bufferization operation would allow you to have an optional result here, though that might mess with your naming scheme.

thanks!
I was originally just going to write a separate op for it (i.e. DSMulOp) but maybe what you've suggested would be better. I guess if I do that I'll just remove the DSS part of the name. perhaps @superlopuh would have some thoughts on it

@alexarice
Copy link
Collaborator

Either would probably work. Which would be better in the long run is hard to tell and probably depends on how you plan to use these operations.

@emmau678 emmau678 marked this pull request as ready for review November 25, 2024 15:40

def __init__(
self,
d: IntRegisterType,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the named argument is better than positional, as it would make sense for me to make this the unallocated type by default, as in the RISC-V equivalent.

@superlopuh
Copy link
Member

I would expect that the two operand case is some sort of assembly shorthand, we could add some custom logic to print it that way when the second argument happens to be the same register as the result, but I don't think we want that reflected in the IR.

@emmau678 emmau678 merged commit 20902a3 into main Nov 26, 2024
15 checks passed
@emmau678 emmau678 deleted the emma/arm-mul-op branch November 26, 2024 11:07
EdmundGoodman pushed a commit to EdmundGoodman/xdsl that referenced this pull request Dec 6, 2024
…ect#3515)

Add mul instruction
(https://developer.arm.com/documentation/ddi0597/2024-06/Base-Instructions/MUL--MULS--Multiply-?lang=en).

The mul instruction supports either 2 or 3 operands (in the case of 2,
the destination is also treated as the 2nd source). This PR only handles
the case where 3 operands are provided, 2 source registers and 1
destination.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialects Changes on the dialects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants