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.

Sign up for GitHub

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

Jump to bottom

asm: add implementations for more ALU operations #10237

Merged
merged 5 commits into from
Feb 15, 2025
Merged

asm: add implementations for more ALU operations #10237

merged 5 commits into from
Feb 15, 2025

Conversation

abrown
Copy link
Contributor

@abrown abrown commented Feb 14, 2025

This adds DSL definitions in the assembler and uses the new instructions for ISLE lowering for the following instructions:

  • add
  • adc
  • or
  • sub
  • sbb
  • xor

The original AluRmiROpcode variants are not yet gone; perhaps in a future pass.

@abrown abrown requested a review from a team as a code owner February 14, 2025 21:50
@abrown abrown requested review from cfallin and removed request for a team February 14, 2025 21:50
@abrown
asm: add implementations for more ALU operations
d737108 
This adds DSL definitions in the assembler _and_ uses the new
instructions for ISLE lowering for the following instructions:
- `add`
- `adc`
- `or`
- `sub`
- `sbb`
- `xor`

The original `AluRmiROpcode` variants are not yet gone; perhaps in a
future pass.
@abrown abrown force-pushed the assembler-alurmir-again branch from 80ab679 to d737108 Compare February 14, 2025 21:51
cfallin
cfallin approved these changes Feb 14, 2025
View reviewed changes
Copy link
Member

@cfallin cfallin 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, thanks!

@abrown abrown requested a review from a team as a code owner February 14, 2025 22:48
@abrown abrown requested review from dicej and removed request for a team February 14, 2025 22:48
@abrown
Copy link
Contributor Author

abrown commented Feb 14, 2025

@cfallin, you might want to take another glance at 9b8aecb; it looks like we're emitting a slightly longer encoding somewhere? Not sure if that's ok...

@abrown abrown requested review from cfallin and removed request for dicej February 14, 2025 22:52
@abrown
review: add rules for smaller encodings
aec34e1 
`cranelift-codegen` would check an immediate size and, if it fit in an
8-bit slot, would use the sign-extending version to encode an
instruction with a 1 byte immediate instead of a 4 byte immediate. This
change adds those extra lowering rules for the new assembler.
@abrown
Copy link
Contributor Author

abrown commented Feb 14, 2025 •
edited
Loading

Ok, looking into this more, we had some extra 8-bit lowering rules up in cranelift-codegen that I hadn't ported to the new assembler--no new instructions needed, just some extra ISLE rules. aec34e1 undoes any extra bytes added in prior disassembly tests and should match what we had in cranelift-codegen previously.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen labels Feb 15, 2025
@cfallin
Copy link
Member

cfallin commented Feb 15, 2025

Updates look good -- thanks for tracking down that divergence!

@cfallin cfallin added this pull request to the merge queue Feb 15, 2025
Merged via the queue into bytecodealliance:main with commit e2f55ab Feb 15, 2025
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfallin cfallin Awaiting requested review from cfallin

Assignees
No one assigned
Labels
cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@abrown @cfallin