x64: refactor the representation of *Mem and *MemImm
#10775
+166
−129
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
abrown
commented
May 14, 2025
| (type GprMemImm extern (enum)) | ||
| (type GprMem extern | ||
| (enum | ||
| (Reg (reg Reg)) |
There was a problem hiding this comment.
Is it now theoretically possible to create a GprMem.Reg in ISLE using the wrong kind of register? If so, we could alter this type to Gpr?
There was a problem hiding this comment.
Would it be possible to type this as (Gpr (reg Gpr))? That more closely matches the semantics of the GprMem constructor in Rust
|
I will say, having tried to use this in #10762, that it doesn't just "solve everything." So we should judge this on whether it will make future matching easier. I know that I've reached for this kind of matching in the past but was frustrated it didn't exist. |
github-actions
bot
added
cranelift
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While reflecting on why #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
RegMemandRegMemImm. We had been wrapping instances of theseReg*types, but this change pushes the variants down to:GprMem,GprMemImm,XmmMem,XmmMemAligned,XmmMemImm, andXmmMemAlignedImm. With this, it should be possible to match directly on the variants in ISLE. This does not change any functionality.