alexcrichton opened PR #6416 from alexcrichton:x64-evex-memory
to bytecodealliance:main
:
Currently load-sinking is enabled for EVEX instructions (aka AVX512 instructions) but the encoding of these instructions is a
todo!()
which can cause a panic for some wasms if the right features are enabled. This commit fills out the support for memory operands in the same manner as VEX-encoded instructions. The main stickler here was that EVEX instructions always use a scaled 8-bit offset which needed extra handling to ensure that the correct offset is emitted.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
alexcrichton has marked PR #6416 as ready for review.
alexcrichton requested abrown for a review on PR #6416.
alexcrichton requested wasmtime-compiler-reviewers for a review on PR #6416.
alexcrichton updated PR #6416.
abrown submitted PR review.
abrown submitted PR review.
abrown created PR review comment:
This doc comment can probably change.
abrown created PR review comment:
I would probably create a private
read
function for this just to avoid confusion.
abrown created PR review comment:
It took me a second to review this because I was worried the inversion would be double-applied or something like that. For reference for others in the future,
EVEX.RXB
are stored in inverted format (do we want to note that here?). I eventually conclude this is actually fine since the only inversions performed are!b
and!x
.
abrown created PR review comment:
I think I'm convinced from the tests that this scaling factor is correct for
vpabs
but I'm not sure this is set up well for the future (or even all of the possible EVEX-encoded instructions that can currently be emitted). Section 2.7.7 talks about the kinds of instructions that do not have embedded broadcast (single scalar result, explicit broadcast in opcode, pure load/store) — these instructions all havescaling != 16
forVL = 128
. I think this means that if any future or current instructions are of this kind, we could get a miscompilation (see the expected scaling factors in table 2-35). I wish there was a way to throw an error of some kind here instead. (The same kind of logic applies for the instructions that _are_ affected by embedded broadcast: instructions could expect scalings of 8 and 4 unless we do more checks, e.g., onEVEX.b
andEVEX.W
).My argument here is "let's prevent errors" when someone tries to add a new EVEX-encoded instruction in the future. I looked into adding a new function to
Avx512Opcode
to signal which tuple type each opcode has but that might not totally work:vpopcnt
, e.g., can be emitted as both "Full Mem" and "Full" (we use the "Full Mem" one, apparently!).vcvtudq2ps
uses "Full", which only usesscaling = 16
whenEVEX.b = 0
andEVEX.W = 1
. MaybeEvexInstruction
needs to gain atuple
field which we force users to specify somehow inemit.rs
. Not sure I have the best solution here but hopefully you get what I mean about protecting against future errors.
abrown created PR review comment:
It's unfortunate that
sink
has to get more specific... I had always hoped this code could be used for some kind of assembler. But this trap is necessary and if we eventually create an assembler this can get refactored.
alexcrichton created PR review comment:
Oh the other main purpose for a
MachBuffer
is this use ofuse_label_at_offset
for rip-relative addressing modes.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Oh my read was that
EvexContext
was what controlled all these bits where whatever setsEvexContext
guides the scaling, but it sounds like that's not right. I see now the listing on each instruction and its "TupleType"!Looking at vpopcnt if we have a separate opcode per-size (which I think we do today anyway) it looks like that means the tuple type is per-opcode still? If that's possible then I could add something like:
impl Avx512Opcode { fn tuple_type(&self) -> Avx512TupleType { /* ... */ } }
which could get fed into the
EvexInstruction
encoder which would then futher handle all theEVEX.{b,W}
cases? That way if more tuple types were added they would result in a non-exhaustive match and require handling?
alexcrichton updated PR #6416.
alexcrichton created PR review comment:
Ok I've attempted to implement this, but a double-check is certainly in order
alexcrichton updated PR #6416.
abrown submitted PR review:
LGTM. From my reading, we now map the tuple type to the values in tables 2-34 and 2-35 for
Full
andFullMem
so we shouldn't have problems with the existing instructions. And any additions toAvx512Opcode
will be forced to updatetuple_type()
.
alexcrichton updated PR #6416.
alexcrichton has enabled auto merge for PR #6416.
alexcrichton merged PR #6416.
Last updated: Dec 23 2024 at 13:07 UTC