Skip to content

x64: Refactor multiplication instructions#7871

Merged
abrown merged 3 commits intobytecodealliance:mainfrom
alexcrichton:x64-mul
Feb 9, 2024
Merged

x64: Refactor multiplication instructions#7871
abrown merged 3 commits intobytecodealliance:mainfrom
alexcrichton:x64-mul

Conversation

@alexcrichton
Copy link
Member

This commit is inspired after reading over some code from #7865 and #7866. The goal of this commit was to refactor scalar multiplication-related instructions in the x64 backend to more closely align with their native instructions. Changes include:

  • The MulHi instruction is renamed to Mul. This represents either mul or imul producing a doublewide result.
  • A Mul8 instruction was added to correspond to Mul for the 8-bit variants that produce a doublewide result in the AX register rather than the other instructions which split between RAX and RDX.
  • The UMulLo instruction was removed as now it's covered by Mul
  • The AluRmiROpcode::Mul opcode was removed in favor of new IMul and IMulImm instructions. Register allocation and emission already had special cases for Mul which felt better as standalone instructions rather than putting in an existing variant.

Lowerings using imul are not affected in general but the IMulImm instruction has different register allocation behavior than before which allows the destination to have a different register than the first operand. The umulhi and smulhi instructions are also reimplemented with their 8-bit variants instead of extension-plus-16-bit variants.

This commit is inspired after reading over some code from bytecodealliance#7865
and bytecodealliance#7866. The goal of this commit was to refactor
scalar multiplication-related instructions in the x64 backend to more
closely align with their native instructions. Changes include:

* The `MulHi` instruction is renamed to `Mul`. This represents either
  `mul` or `imul` producing a doublewide result.
* A `Mul8` instruction was added to correspond to `Mul` for the 8-bit
  variants that produce a doublewide result in the `AX` register rather
  than the other instructions which split between `RAX` and `RDX`.
* The `UMulLo` instruction was removed as now it's covered by `Mul`
* The `AluRmiROpcode::Mul` opcode was removed in favor of new `IMul` and
  `IMulImm` instructions. Register allocation and emission already had
  special cases for `Mul` which felt better as standalone instructions
  rather than putting in an existing variant.

Lowerings using `imul` are not affected in general but the `IMulImm`
instruction has different register allocation behavior than before which
allows the destination to have a different register than the first
operand. The `umulhi` and `smulhi` instructions are also reimplemented
with their 8-bit variants instead of extension-plus-16-bit variants.
@alexcrichton alexcrichton requested a review from a team as a code owner February 5, 2024 17:44
@alexcrichton alexcrichton requested review from abrown and removed request for a team February 5, 2024 17:44
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen labels Feb 5, 2024
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Nice!

@fitzgen fitzgen enabled auto-merge February 6, 2024 22:49
@fitzgen fitzgen disabled auto-merge February 6, 2024 22:49
These are all covered by the filetests framework now too.
@alexcrichton
Copy link
Member Author

cc @abrown you mentioned that you were also taking a look at this, want me to hold off on merging for that?

@github-actions github-actions bot added the winch Winch issues or pull requests label Feb 9, 2024
@github-actions
Copy link

github-actions bot commented Feb 9, 2024

Subscribe to Label Action

cc @saulecabrera

Details This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "winch"

Thus the following users have been cc'd because of the following labels:

  • saulecabrera: winch

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@abrown
Copy link
Member

abrown commented Feb 9, 2024

Nah, it's good to go now that @fitzgen looked at it.

@abrown abrown added this pull request to the merge queue Feb 9, 2024
Merged via the queue into bytecodealliance:main with commit 1f6e901 Feb 9, 2024
@alexcrichton alexcrichton deleted the x64-mul branch February 10, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator winch Winch issues or pull requests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants