wasmtime / PR #11153 x64: EVEX Encoding for new assembler · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #11153 x64: EVEX Encoding for new assembler

Wasmtime GitHub notifications bot (Jun 27 2025 at 21:46):

rahulchaphalkar opened PR #11153 from rahulchaphalkar:pr-evex-encoding to bytecodealliance:main:

Adds evex encoding for new assembler.
Adds vaddpd evex instr.

Following items are not implemented in this PR / left out intentionally -

Broadcast attribute (bcst). Will be implemented in future pr.

Features: Uses avx512vl feature right now, this will be changed to AND of avx512vl and avx512f, and at a later point to avx10.1, avx10.2 etc.

vaddpd is not lowered yet

@abrown Ready for review. I'll add a few code comments for some discussion soon, apart from above list of omitted items.

Wasmtime GitHub notifications bot (Jun 27 2025 at 21:46):

rahulchaphalkar requested alexcrichton for a review on PR #11153.

Wasmtime GitHub notifications bot (Jun 27 2025 at 21:46):

rahulchaphalkar requested wasmtime-compiler-reviewers for a review on PR #11153.

Wasmtime GitHub notifications bot (Jun 27 2025 at 22:28):

alexcrichton requested abrown for a review on PR #11153.

Wasmtime GitHub notifications bot (Jun 27 2025 at 22:48):

rahulchaphalkar submitted PR review.

Wasmtime GitHub notifications bot (Jun 27 2025 at 22:48):

rahulchaphalkar created PR review comment:

EvexLength and VexLength enums have members with same names, causing me to specify which enum. I can either

Make all vex/evex look the same form, like VexLength::L128 and EvexLength::L128 everywhere

Introduce constants like V128, E128 etc.

Use VexLength for evex as well or add a common VectorLength enum for both vex/evex.
Something else?

Wasmtime GitHub notifications bot (Jul 07 2025 at 23:22):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2025 at 23:22):

abrown created PR review comment:

As a further point on my comment above about Operand::mask_reg, I think we would also want Operand::zeroing to be a runtime decision. In other words, when a user of this assembler is encoding an instruction, they should have the ability to decide not only which opmask register to use (k0-k7) but also to select if they want the "merging-masking" or "zeroing-masking" behavior (section 2.7.4 in the manual). So this bool field would need to be present on the generated instruction struct, not here in the DSL.

This raises some questions, though, about how to handle these "options" on an instruction:

for opmask registers, we probably want some way to use k0 by default, which does no masking at all; other, than this, opmask registers act pretty much like other operands (no regalloc, though)

for masking behavior, we do not want them to act like other operands — this flag isn't an operand at all! I propose we encode the default behavior in EVEX.z and open an issue to figure out a good way to expose this kind of flag on instructions. Why? Because...

for vector lengths, we actually want the ability to select the length as one of these flags (L128, L256, L512). Right now we fix this in the DSL but, if we had a way to expose flags, we would want instruction lengths to parameterize the instruction.

But all of that does not need to be figured out in this PR. I propose we just default to using k0 and ignore the masking behavior here and open an issue to solve this later.

Wasmtime GitHub notifications bot (Jul 07 2025 at 23:22):

abrown created PR review comment:

This doesn't make sense because the opmask registers are true operands. The user can specify which register (k0-k7) to use when the assembler is used at runtime, so we can't predetermine which mask_reg the instruction should be using here when instructions are defined.

Wasmtime GitHub notifications bot (Jul 07 2025 at 23:22):

abrown created PR review comment:

I think we should just call it Length and have it contain all three: L128, L256, and L512. Then, in the validation for VEX instructions we should only allow L128 and L256.

Wasmtime GitHub notifications bot (Jul 07 2025 at 23:22):

abrown created PR review comment:

I'm not sure that we want to copy-paste these yet from the VEX implementation. I suspect they all are the same but I'm not really sure, so we might want to start with only the formats we need and build from there. At some later point we could even deduplicate between VEX and EVEX.

Wasmtime GitHub notifications bot (Jul 07 2025 at 23:22):

abrown created PR review comment:

Guess the r() is missing from a bunch of VEX instructions which goes to show how little it changes!

Wasmtime GitHub notifications bot (Jul 07 2025 at 23:22):

abrown created PR review comment:

        mmm: u8,
The EVEX field is 3-bit: EVEX.mmm. Makes me think we should check that we're using this u8 correctly elsewhere.

Wasmtime GitHub notifications bot (Jul 07 2025 at 23:22):

abrown created PR review comment:

E.g., this is true for now but there are two other decoding map IDs, 5 and 6, that would trip over this.

Wasmtime GitHub notifications bot (Jul 07 2025 at 23:22):

abrown created PR review comment:

If we ignore the opmask registers and masking behavior then we don't need this, right? Or I guess we still need to print {k0} or something like that to match Capstone? Maybe we should implement the "opmask registers as operands" logic after all...

Wasmtime GitHub notifications bot (Jul 07 2025 at 23:22):

abrown created PR review comment:

        inst("addpd", fmt("A", [rw(xmm1), r(align(xmm_m128))]), rex([0x66, 0x0F, 0x58]).r(), _64b | compat | sse2).alt(avx, "vaddpd_b"),
        inst("vaddpd", fmt("B", [w(xmm1), r(xmm2), r(xmm_m128)]), vex(L128)._66()._0f().op(0x58).r(), _64b | compat | avx),

The order doesn't matter to the DSL but we should keep it consistent with the manual.

Wasmtime GitHub notifications bot (Jul 07 2025 at 23:23):

abrown commented on PR #11153:

I'll probably take over this PR soon since @rahulchaphalkar is out for a bit.

Wasmtime GitHub notifications bot (Jul 16 2025 at 20:25):

abrown updated PR #11153.

Wasmtime GitHub notifications bot (Jul 16 2025 at 20:53):

abrown updated PR #11153.

Wasmtime GitHub notifications bot (Jul 16 2025 at 20:56):

abrown updated PR #11153.

Wasmtime GitHub notifications bot (Jul 16 2025 at 21:02):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2025 at 21:02):

abrown created PR review comment:

I'll rename this to WBit or something like that...

Wasmtime GitHub notifications bot (Jul 16 2025 at 21:02):

abrown created PR review comment:

This logic should be (AVX512VL AND AVX512F). Additionally, since we need the ability to AND things together, we need to teach the whole expression to mean: (_64b | compat) & (...) (maybe in another PR, since this affects many instructions beyond just this one).

Wasmtime GitHub notifications bot (Jul 16 2025 at 21:26):

abrown updated PR #11153.

Wasmtime GitHub notifications bot (Jul 16 2025 at 21:30):

abrown updated PR #11153.

Wasmtime GitHub notifications bot (Jul 16 2025 at 21:44):

abrown updated PR #11153.

Wasmtime GitHub notifications bot (Jul 16 2025 at 21:48):

abrown updated PR #11153.

Wasmtime GitHub notifications bot (Jul 16 2025 at 21:56):

abrown updated PR #11153.

Wasmtime GitHub notifications bot (Jul 16 2025 at 22:19):

abrown commented on PR #11153:

@cfallin or @alexcrichton: can one of you review this? I've changed this PR substantially since it was first opened so I think it needs a new reviewer that is not me.

Wasmtime GitHub notifications bot (Jul 16 2025 at 22:19):

abrown requested alexcrichton for a review on PR #11153.

Wasmtime GitHub notifications bot (Jul 16 2025 at 22:19):

abrown requested cfallin for a review on PR #11153.

Wasmtime GitHub notifications bot (Jul 16 2025 at 22:20):

cfallin commented on PR #11153:

Sure, taking a look!

Wasmtime GitHub notifications bot (Jul 16 2025 at 22:26):

cfallin submitted PR review:

LGTM. Taking your word (and the fuzzer's) on the actual prefix-encoding details!

Wasmtime GitHub notifications bot (Jul 16 2025 at 22:36):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2025 at 22:36):

alexcrichton created PR review comment:

I think this may actually work out? Not to say our current modeling of things is correct, but functionally with | avx512f here I think it has the net effect of requiring more features. Notably we take all the features the assembler says and flag them all as requirements of each instruction, so by listing more features here it's interpreted as effectively requiring all of them. I don't know if that's an accurate reflection of _64b and compat though as we otherwise basically ignore those features right now.

Although rereading your comment now I suspect you're talking about the interactions of _64b and compat with feature flags, which is exactly the gray area for me that iunno much about, so probably just disregard me

Wasmtime GitHub notifications bot (Jul 16 2025 at 22:51):

cfallin merged PR #11153.

Last updated: Dec 06 2025 at 07:03 UTC