Stream: git-wasmtime

Topic: wasmtime / PR #10754 Initial support for vex encoding for...

Wasmtime GitHub notifications bot (May 08 2025 at 23:05):

jlb6740 requested abrown for a review on PR #10754.

Wasmtime GitHub notifications bot (May 08 2025 at 23:05):

jlb6740 requested wasmtime-compiler-reviewers for a review on PR #10754.

Wasmtime GitHub notifications bot (May 08 2025 at 23:05):

jlb6740 opened PR #10754 from jlb6740:add-initial-vex-encoding to bytecodealliance:main:

<!--
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.html

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

Wasmtime GitHub notifications bot (May 08 2025 at 23:10):

jlb6740 commented on PR #10754:

The review for this patch started here https://github.com/bytecodealliance/wasmtime/pull/10695. This attempts to add support for vex encoding and in the process adds the lowering of couple of initial vex instructions vaddpd and vaddps.
@rahulchaphalkar

Wasmtime GitHub notifications bot (May 09 2025 at 02:31):

jlb6740 updated PR #10754.

Wasmtime GitHub notifications bot (May 09 2025 at 16:21):

jlb6740 updated PR #10754.

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

I'm still not convinced we need these yet. Maybe we should wait to add YMM and ZMM when we actually need those instructions.

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

For printing VexPP and VexMMMMM, if we print using . instead of _ then we can match the manual a bit more closely. E.g., VEX.128.0F.WIG 58 /r.

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

Nit: move the xmm_* locations together?

            rm8 | rm16 | rm32 | rm64 | m8 | m16 | m32 | m64 | xmm_m32 | xmm_m64 | xmm_m128

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

If this is for VEX encoding maybe it should go with the VEX-related code. Plus, don't we need to check the actual values in each group?

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown submitted PR review:

There's some smaller review comments mixed but I wanted to highlight the VexInstruction encoding as the only major thing left to resolve; otherwise this looks good to me. I left some comments here about that encoding part but we can talk offline in more detail.

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

Let's not make this public if we don't have to.

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

We should not create default registers.

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

You can use the following to manually format the instruction definition on a single line.

#[rustfmt::skip] // Keeps instructions on a single line.
pub fn list() -> Vec<Inst> {

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

Let's not make these public if we don't have to.

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

Might as well inline generate_vex here since there are no other uses.

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

This still seems unaddressed from the previous PR: let's look for a way to avoid creating defaults that we later overwrite (or forget to overwrite!).

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

But let's just keep these in add.rs for now; if we want to reorganize every instruction into separate files later, we can as a separate refactor.

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

The manual says 00000 is reserved for the m-mmmm field and would result in a #UD. Maybe we should remove None since we can't use it (?).

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

I don't think this (or reg) is quite right: we won't always have a memory operand here. It would seem we need to do the REX flags encoding, extract the R, X, and B fields, and pass those around here.

There are instructions like the following that we won't be able to handle with this approach:

• VMOVHLPS xmm1, xmm2, xmm3
• VBLENDVPS xmm1, xmm2, xmm3/m128, xmm4

If we alter the VexInstruction thing to just emit the VEX _prefixes_, not the whole instruction, then I think things will work better here (no more default instructions) and will be more flexible for the future. Plus, we can reuse all of the suffix stuff that should be the same (see encode_rex_suffixes).

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

The only possibilities for the pp field are: None, 66, F3, and F2. We can pare this down and rename it Pp or Prefix?

Wasmtime GitHub notifications bot (May 09 2025 at 17:11):

abrown created PR review comment:

Let's set up a match statement over the operands like the one that exists for REX. This code only handles one specific pattern, [xmm1, xmm2, xmm_m128], but there are others. We can leave a _ => unimplemented!() so it is clear that we just haven't worked on the other patterns yet.

Wasmtime GitHub notifications bot (May 09 2025 at 17:12):

abrown created PR review comment:

In fact, do we even use this?

Wasmtime GitHub notifications bot (May 09 2025 at 17:12):

abrown submitted PR review.

Wasmtime GitHub notifications bot (May 09 2025 at 17:13):

abrown submitted PR review.

Wasmtime GitHub notifications bot (May 09 2025 at 17:13):

abrown created PR review comment:

We might not use this either in the DSL phase; isn't this only necessary when we encode in the cranelift-assembler-x64 crate?

Wasmtime GitHub notifications bot (May 09 2025 at 22:10):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 09 2025 at 22:10):

jlb6740 created PR review comment:

I think the underscore is needed because this naming is not just used for printing, but it also used as part of syntax for an associated item:

vex.vvvv = Some(self.xmm2.enc()); // cranelift/assembler-x64/meta/src/generate/format.rs:135
        vex.prefix = LegacyPrefix::.66; // cranelift/assembler-x64/meta/src/generate/format.rs:136
        vex.map = OpcodeMap::_0F; // cranelift/assembler-x64/meta/src/generate/format.rs:137

where here the LegacyPrefix::.66 is not valid syntax.

Wasmtime GitHub notifications bot (May 09 2025 at 22:10):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (May 09 2025 at 22:12):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 09 2025 at 22:12):

jlb6740 created PR review comment:

Maybe I can get around this. Will see.

Wasmtime GitHub notifications bot (May 10 2025 at 04:33):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 10 2025 at 04:33):

jlb6740 created PR review comment:

I am not really sure what to do here other than duplicating it? The primary encode function in vex.rs calls this directly. I don't want to try to refactor or move that code as it is well tested as the same as in the current codegen and it doesn't belong in mem.rs. The emit_modrm_sib_disp is common code to rex and vex and I assume evex encoding and it was public. Being common to multiple encodings, what's the motivation for making it private?

Wasmtime GitHub notifications bot (May 10 2025 at 04:51):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (May 10 2025 at 05:50):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 10 2025 at 05:50):

jlb6740 created PR review comment:

None is used as default value when creating a VexInstruction. I could change that to be an option, but the way it is now is the exact same as the current implementation in the codegen encoding:

https://github.com/bytecodealliance/wasmtime/blob/main/cranelift/codegen/src/isa/x64/encoding/vex.rs#L27-L41

Do you think we should change this to be an option?

Wasmtime GitHub notifications bot (May 12 2025 at 23:07):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 12 2025 at 23:07):

jlb6740 created PR review comment:

No .. will remove. I believe it remained as an artifact of a rebase when in fact it could/should have been purged at that time.

Wasmtime GitHub notifications bot (May 14 2025 at 00:19):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 14 2025 at 00:19):

jlb6740 created PR review comment:

Yes, I believe this is copied over or merged from the codegen. I called this vexPP in the DSL. Will do the same here.

Wasmtime GitHub notifications bot (May 19 2025 at 21:26):

jlb6740 updated PR #10754.

Wasmtime GitHub notifications bot (May 19 2025 at 23:59):

jlb6740 updated PR #10754.

Wasmtime GitHub notifications bot (May 20 2025 at 00:01):

jlb6740 updated PR #10754.

Wasmtime GitHub notifications bot (May 20 2025 at 00:14):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 20 2025 at 00:14):

jlb6740 created PR review comment:

Yes .. could not remove but any suggestions to overcome the "expected identifier" error welcomed.

Wasmtime GitHub notifications bot (May 20 2025 at 00:40):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 20 2025 at 00:40):

jlb6740 created PR review comment:

Got rid of the default method and removed this.

Wasmtime GitHub notifications bot (May 20 2025 at 01:37):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 20 2025 at 01:37):

jlb6740 created PR review comment:

Let's discuss this further. I am not sure what is being recommended here but it I think you are suggesting a different approach than using the current vex emit function? I need to understand exactly what is meant here, but can we iterate on this recommendation?

Wasmtime GitHub notifications bot (May 20 2025 at 01:39):

jlb6740 commented on PR #10754:

@abrown Hi this is ready for second review. Everything with a +1 I believe is addressed and everything with a confuse emoji means it is not addressed due to whatever is written in the comment that follows.

Wasmtime GitHub notifications bot (May 20 2025 at 01:39):

jlb6740 deleted a comment on PR #10754:

@abrown Hi this is ready for second review. Everything with a +1 I believe is addressed and everything with a confuse emoji means it is not addressed due to whatever is written in the comment that follows.

Wasmtime GitHub notifications bot (May 20 2025 at 01:39):

jlb6740 commented on PR #10754:

@abrown Hi, thanks for all the very good comments. This is ready for second review. Everything with a +1 I believe is addressed and everything with a confuse emoji means it is not addressed due to whatever is written in the comment that follows.

Wasmtime GitHub notifications bot (May 20 2025 at 01:40):

jlb6740 deleted a comment on PR #10754:

@abrown Hi, thanks for all the very good comments. This is ready for second review. Everything with a +1 I believe is addressed and everything with a confuse emoji means it is not addressed due to whatever is written in the comment that follows.

Wasmtime GitHub notifications bot (May 20 2025 at 01:44):

jlb6740 updated PR #10754.

Wasmtime GitHub notifications bot (May 20 2025 at 16:06):

abrown submitted PR review.

Wasmtime GitHub notifications bot (May 20 2025 at 16:06):

abrown created PR review comment:

If you write!(f, "66") here, then we can add whatever prefix we want later, fmtln!(f, "_{}", vex.pp) or fmtln!(f, ".{}", vex.pp).

Wasmtime GitHub notifications bot (May 20 2025 at 16:08):

abrown submitted PR review.

Wasmtime GitHub notifications bot (May 20 2025 at 16:08):

abrown created PR review comment:

I don't think we need this, right?

Wasmtime GitHub notifications bot (May 22 2025 at 23:52):

jlb6740 updated PR #10754.

Wasmtime GitHub notifications bot (May 22 2025 at 23:53):

jlb6740 updated PR #10754.

Wasmtime GitHub notifications bot (May 23 2025 at 00:42):

jlb6740 updated PR #10754.

Wasmtime GitHub notifications bot (May 23 2025 at 01:33):

jlb6740 updated PR #10754.

Wasmtime GitHub notifications bot (May 23 2025 at 01:45):

jlb6740 updated PR #10754.

Wasmtime GitHub notifications bot (May 23 2025 at 01:50):

jlb6740 updated PR #10754.

Wasmtime GitHub notifications bot (May 23 2025 at 02:12):

jlb6740 commented on PR #10754:

@abrown Hi, thanks for all the great comments. This is ready for second review. Everything with a +1 I believe is addressed and everything with a confuse emoji means it is not addressed due to whatever commentary follows.

Wasmtime GitHub notifications bot (May 23 2025 at 21:21):

abrown submitted PR review:

Let's merge this; I'll continue working on this to refine the encoding logic so it will work with more instructions. Thanks!

Wasmtime GitHub notifications bot (May 23 2025 at 22:37):

abrown updated PR #10754.

Wasmtime GitHub notifications bot (May 23 2025 at 22:37):

abrown has enabled auto merge for PR #10754.

Wasmtime GitHub notifications bot (May 23 2025 at 23:12):

abrown merged PR #10754.

Last updated: Dec 06 2025 at 07:03 UTC