Wasmtime GitHub notifications bot (May 19 2023 at 20:57): Wasmtime GitHub notifications bot (May 19 2023 at 20:57):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
-->
Wasmtime GitHub notifications bot (May 19 2023 at 20:57):alexcrichton has marked PR #6416 as ready for review.
Wasmtime GitHub notifications bot (May 19 2023 at 20:57):alexcrichton requested abrown for a review on PR #6416.
Wasmtime GitHub notifications bot (May 19 2023 at 20:57):alexcrichton requested wasmtime-compiler-reviewers for a review on PR #6416.
Wasmtime GitHub notifications bot (May 19 2023 at 21:45):alexcrichton updated PR #6416.
Wasmtime GitHub notifications bot (May 22 2023 at 23:57):abrown submitted PR review.
Wasmtime GitHub notifications bot (May 22 2023 at 23:57):abrown submitted PR review.
Wasmtime GitHub notifications bot (May 22 2023 at 23:57):abrown created PR review comment:
This doc comment can probably change.
Wasmtime GitHub notifications bot (May 22 2023 at 23:57):abrown created PR review comment:
I would probably create a private
readfunction for this just to avoid confusion.
Wasmtime GitHub notifications bot (May 22 2023 at 23:57):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.RXBare stored in inverted format (do we want to note that here?). I eventually conclude this is actually fine since the only inversions performed are!band!x.
Wasmtime GitHub notifications bot (May 22 2023 at 23:57):abrown created PR review comment:
I think I'm convinced from the tests that this scaling factor is correct for
vpabsbut 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 != 16forVL = 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.bandEVEX.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
Avx512Opcodeto 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!).vcvtudq2psuses "Full", which only usesscaling = 16whenEVEX.b = 0andEVEX.W = 1. MaybeEvexInstructionneeds to gain atuplefield 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.
Wasmtime GitHub notifications bot (May 22 2023 at 23:57):abrown created PR review comment:
It's unfortunate that
sinkhas 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.
Wasmtime GitHub notifications bot (May 23 2023 at 13:35):alexcrichton created PR review comment:
Oh the other main purpose for a
MachBufferis this use ofuse_label_at_offsetfor rip-relative addressing modes.
Wasmtime GitHub notifications bot (May 23 2023 at 13:43):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (May 23 2023 at 13:43):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (May 23 2023 at 13:43):alexcrichton created PR review comment:
Oh my read was that
EvexContextwas what controlled all these bits where whatever setsEvexContextguides 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
EvexInstructionencoder 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?
Wasmtime GitHub notifications bot (May 23 2023 at 14:20):alexcrichton updated PR #6416.
Wasmtime GitHub notifications bot (May 23 2023 at 14:22):alexcrichton created PR review comment:
Ok I've attempted to implement this, but a double-check is certainly in order
Wasmtime GitHub notifications bot (May 23 2023 at 14:58):alexcrichton updated PR #6416.
Wasmtime GitHub notifications bot (May 24 2023 at 16:23):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
FullandFullMemso we shouldn't have problems with the existing instructions. And any additions toAvx512Opcodewill be forced to updatetuple_type().
Wasmtime GitHub notifications bot (May 24 2023 at 18:30):alexcrichton updated PR #6416.
Wasmtime GitHub notifications bot (May 24 2023 at 18:30):alexcrichton has enabled auto merge for PR #6416.
Wasmtime GitHub notifications bot (May 24 2023 at 19:32):alexcrichton merged PR #6416.
Last updated: Nov 22 2024 at 17:03 UTC