Stream: git-wasmtime

Topic: wasmtime / PR #6416 x64: Add memory operand support to EV...


view this post on Zulip 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:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2023 at 20:57):

alexcrichton has marked PR #6416 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2023 at 20:57):

alexcrichton requested abrown for a review on PR #6416.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2023 at 20:57):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #6416.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2023 at 21:45):

alexcrichton updated PR #6416.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2023 at 23:57):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2023 at 23:57):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2023 at 23:57):

abrown created PR review comment:

This doc comment can probably change.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2023 at 23:57):

abrown created PR review comment:

I would probably create a private read function for this just to avoid confusion.

view this post on Zulip 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.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.

view this post on Zulip 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 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 have scaling != 16 for VL = 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., on EVEX.b and EVEX.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 uses scaling = 16 when EVEX.b = 0 and EVEX.W = 1. Maybe EvexInstruction needs to gain a tuple field which we force users to specify somehow in emit.rs. Not sure I have the best solution here but hopefully you get what I mean about protecting against future errors.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2023 at 23:57):

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.

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 13:35):

alexcrichton created PR review comment:

Oh the other main purpose for a MachBuffer is this use of use_label_at_offset for rip-relative addressing modes.

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 13:43):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 13:43):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 13:43):

alexcrichton created PR review comment:

Oh my read was that EvexContext was what controlled all these bits where whatever sets EvexContext 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 the EVEX.{b,W} cases? That way if more tuple types were added they would result in a non-exhaustive match and require handling?

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 14:20):

alexcrichton updated PR #6416.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 14:58):

alexcrichton updated PR #6416.

view this post on Zulip 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 Full and FullMem so we shouldn't have problems with the existing instructions. And any additions to Avx512Opcode will be forced to update tuple_type().

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 18:30):

alexcrichton updated PR #6416.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 18:30):

alexcrichton has enabled auto merge for PR #6416.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 19:32):

alexcrichton merged PR #6416.


Last updated: Dec 23 2024 at 13:07 UTC