Skip to content

x64: convert some SSE shift instructions #10762

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

Merged
merged 1 commit into from
May 15, 2025

Conversation

abrown
Copy link
Member

@abrown abrown commented May 9, 2025

No description provided.

@abrown abrown requested a review from a team as a code owner May 9, 2025 22:26
@abrown abrown requested review from cfallin and removed request for a team May 9, 2025 22:26
Comment on lines +1038 to +1043
fn is_imm8_xmm(&mut self, src: &XmmMemImm) -> Option<u8> {
match src.clone().to_reg_mem_imm() {
RegMemImm::Imm { simm32 } => Some(u8::try_from(simm32).ok()?),
_ => None,
}
}
Copy link
Member Author

@abrown abrown May 9, 2025

Choose a reason for hiding this comment

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

I don't really want to add this but, because of the type signatures of the inst.isle constructors, I have to find a way to match on a XmmMemImm instead of GprMemImm. One alternative to adding this would be to have is_imm8 examine the more general RegMemImm instead, but this would mean the constructor types would have to change. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Poking around it looks like these shifting instructions are the only ones using XmmMemImm, so perhaps we could drop that type entirely? All the various helpers seem to be using mask_xmm_shift and the current lowerings could dispatch to a helper taking the RegMemImm result of mask_xmm_shift which handles the *Imm case to delegate to different constructors than when in the Reg or Mem case?

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen isle Related to the ISLE domain-specific language labels May 10, 2025
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

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

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

  • cfallin: isle
  • fitzgen: isle

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

Learn more.

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.

LGTM with Alex's suggestion -- thanks!

abrown added a commit to abrown/wasmtime that referenced this pull request May 14, 2025
While reflecting on why bytecodealliance#10762 is hard, it seemed that the underlying
problem to many ISLE matching issues is our inability to actually
inspect the variants of the register specific versions of `RegMem` and
`RegMemImm`. We had been wrapping instances of these `Reg*` types, but
this change pushes the variants down to: `GprMem`, `GprMemImm`,
`XmmMem`, `XmmMemAligned`, `XmmMemImm`, and `XmmMemAlignedImm`. With
this, it should be possible to match directly on the variants in ISLE.
This does not change any functionality.
@alexcrichton
Copy link
Member

I can also clarify I'm happy to see this landed as-is. @abrown mentioned he wanted to chat with me about my suggestion but I'm busy all this week and won't be back until next week so I think it's ok to land this as-is and follow-up as necessary if you'd like to @abrown

@alexcrichton
Copy link
Member

that was the wrong button...

@alexcrichton alexcrichton reopened this May 14, 2025
@abrown abrown added this pull request to the merge queue May 15, 2025
Merged via the queue into bytecodealliance:main with commit 8d853cc May 15, 2025
106 checks passed
@abrown abrown deleted the asm-sse-shift branch May 15, 2025 00:59
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 isle Related to the ISLE domain-specific language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants