Stream: git-wasmtime

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

Wasmtime GitHub notifications bot (Apr 29 2025 at 01:34):

jlb6740 opened PR #10695 from jlb6740:add-initial-vex-encoding-support-1-of-2 to bytecodealliance:main:

Patch is first, one or two more will be pushed for enabling.

<!--
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 (Apr 29 2025 at 01:34):

jlb6740 requested abrown for a review on PR #10695.

Wasmtime GitHub notifications bot (Apr 29 2025 at 01:34):

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

Wasmtime GitHub notifications bot (Apr 29 2025 at 01:34):

jlb6740 requested alexcrichton for a review on PR #10695.

Wasmtime GitHub notifications bot (Apr 29 2025 at 01:34):

jlb6740 requested wasmtime-core-reviewers for a review on PR #10695.

Wasmtime GitHub notifications bot (Apr 29 2025 at 16:13):

jlb6740 updated PR #10695.

Wasmtime GitHub notifications bot (Apr 29 2025 at 17:41):

jlb6740 updated PR #10695.

Wasmtime GitHub notifications bot (Apr 29 2025 at 17:47):

jlb6740 updated PR #10695.

Wasmtime GitHub notifications bot (Apr 29 2025 at 21:04):

jlb6740 updated PR #10695.

Wasmtime GitHub notifications bot (Apr 30 2025 at 18:40):

jlb6740 updated PR #10695.

Wasmtime GitHub notifications bot (Apr 30 2025 at 18:45):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 30 2025 at 18:45):

alexcrichton created PR review comment:

Would it be possible to duplicate necessary types here? The intent is that the meta crate is only used at build-time and isn't a runtime dependency but this is otherwise adding the meta crate as a runtime dependency as well

Wasmtime GitHub notifications bot (Apr 30 2025 at 18:53):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 30 2025 at 18:53):

cfallin created PR review comment:

In the cranelift-codegen cinematic universe of crates, we have -meta and -shared; the -shared crate is depended on by both the meta and main crate. Perhaps the same split makes sense here?

Wasmtime GitHub notifications bot (Apr 30 2025 at 20:29):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Apr 30 2025 at 20:29):

jlb6740 created PR review comment:

Hi @alexcrichton, I think so. I did it this way as it seemed to be required for a previous iteration of this patch set, plus it assures consistency and avoids duplication, but I understand we don't want a dependency on the meta crate and that duplication is ok. If I do believe there is a dependency we want to keep, I'll follow @cfallin advise and see if a shared crate makes sense here. Also note, there are a few unnecessary elements in the vex struct in the meta crate that I am removing along with commented out code intended to help develop the follow-up patch 2 of 2 to be submitted later when we enable this.

Wasmtime GitHub notifications bot (Apr 30 2025 at 22:34):

jlb6740 updated PR #10695.

Wasmtime GitHub notifications bot (Apr 30 2025 at 22:36):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Apr 30 2025 at 22:36):

jlb6740 created PR review comment:

@alexcrichton .. I believe this is addressed now.

Wasmtime GitHub notifications bot (May 01 2025 at 01:18):

abrown submitted PR review:

I got about halfway through reviewing this; here are some comments.

Wasmtime GitHub notifications bot (May 01 2025 at 01:18):

abrown created PR review comment:

I think at the DSL level we may want to have separate booleans for each bit, e.g.:

r: bool,
x: bool,
b: bool,
w: bool,

As it stands, this duplicates the r bit.

Wasmtime GitHub notifications bot (May 01 2025 at 01:18):

abrown created PR review comment:

This is the same as rm128... I'm not convinced adding these unused variants is necessary at this point, either.

Wasmtime GitHub notifications bot (May 01 2025 at 01:18):

abrown created PR review comment:

It will be helpful to check here for invalid instruction definitions: that way we can find them early, at codegen time.

Wasmtime GitHub notifications bot (May 01 2025 at 01:18):

abrown created PR review comment:

We shouldn't need to create a default (i.e., invalid) VEX instruction ever, right?

Wasmtime GitHub notifications bot (May 01 2025 at 01:18):

abrown created PR review comment:

This duplicates one of the definitions in add.rs, right? Let's just keep all the addition in that file to avoid this kind of mistake.

Wasmtime GitHub notifications bot (May 01 2025 at 01:18):

abrown created PR review comment:

This is not right:

        self.bits() / 8

Wasmtime GitHub notifications bot (May 01 2025 at 01:18):

abrown created PR review comment:

        // test a single input, append `.seed(0x<failing seed>)`.

Wasmtime GitHub notifications bot (May 01 2025 at 01:18):

abrown created PR review comment:

This seems a bit redundant to match here in any case...

Wasmtime GitHub notifications bot (May 01 2025 at 01:18):

abrown created PR review comment:

When you're adding brace-delimited blocks, you can use Formatter::add_block to get the indentation right, avoid forgetting to add a closing brace, and to match the rest of this crate:

        f.add_block("match &self.xmm_m128", |f| {
            fmtln!(f, "XmmMem::Xmm(r) => {{vex.rm =  XmmMem::Xmm(r.clone());}}");
            fmtln!(f, "XmmMem::Mem(m) => {{vex.rm =  XmmMem::Mem(m.clone());}}");
        });

Wasmtime GitHub notifications bot (May 01 2025 at 01:18):

abrown created PR review comment:

Instead of creating a default (i.e., invalid) instruction here, why not use a new constructor or a builder pattern (e.g., op(...).reg(...)...) to create only valid instructions?

Wasmtime GitHub notifications bot (May 01 2025 at 02:49):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 01 2025 at 02:49):

jlb6740 created PR review comment:

Yes, I am not positive, but I don't even think any of these bits will need to be set at the DSL level. For now I'll just remove the rxb and add the b and w bits as suggested here (but maybe we can remove later).

Wasmtime GitHub notifications bot (May 01 2025 at 02:58):

jlb6740 created PR review comment:

For the final vex support I removed the one in add.rs (but put it back for this pr). The reason is simple, I was organizing the instructions the same way they are organized in the manual by opcode (specifically Vol 2A 3-34). The ADDPD opcode instruction section covers rex, vex, and evex. What do you think about this organization of grouping instructions consistent with the manual?

Wasmtime GitHub notifications bot (May 01 2025 at 02:58):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 01 2025 at 02:59):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (May 01 2025 at 05:52):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 01 2025 at 05:52):

jlb6740 created PR review comment:

I don't think so. Will remove this.

Wasmtime GitHub notifications bot (May 01 2025 at 06:01):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 01 2025 at 06:01):

jlb6740 created PR review comment:

Yes agreed .. can we implement this validate in the next PR where we turn the plumbing on and we can see that the validation is actually doing the right thing against those first few instructions or would you like to see a little more of this filled out now?

Wasmtime GitHub notifications bot (May 01 2025 at 06:25):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 01 2025 at 06:25):

jlb6740 created PR review comment:

I was wanting to be consistent with the manual here:

ADDPD xmm1, xmm2/m128
VADDPD xmm1, xmm2, xmm3/m128

and writing something like this:

inst(
            "vaddpd",
            fmt("B", [w(xmm1), r(xmm2), r(xmm_m128)]),
            vex(0x58).length(_128).pp(_66).mmmmm(_OF),
            _64b | compat | sse,
        ),

instead of like this:

inst(
            "vaddpd",
            fmt("B", [w(xmm1), r(xmm2), r(rm128)]),
            vex(0x58).length(_128).pp(_66).mmmmm(_OF),
            _64b | compat | sse,
        ),

I kept the rm128 because I didn't know if it would fly during review that I replaced in in all the rex encoded instructions but using the naming xmm_m128 is more consistent with the manual. What do you think?

Wasmtime GitHub notifications bot (May 01 2025 at 16:59):

jlb6740 edited PR review comment.

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

jlb6740 closed without merge PR #10695.

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

jlb6740 commented on PR #10695:

Hi @cfallin @alexcrichton @abrown I've attempted to address all comments or at least touch base with the reviewer offline. I've moved this PR to https://github.com/bytecodealliance/wasmtime/pull/10754 where instead of doing this in 2 parts, well just attempt to switch on the lowering of vaddpd and vaddps. I'll close this PR.

Last updated: Dec 06 2025 at 07:03 UTC