Stream: git-wasmtime

Topic: wasmtime / PR #10942 x64: generate rules to pick between ...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 05 2025 at 22:22):

abrown requested wasmtime-compiler-reviewers for a review on PR #10942.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 05 2025 at 22:22):

abrown requested cfallin for a review on PR #10942.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 05 2025 at 22:22):

abrown opened PR #10942 from abrown:asm-auto-alt to bytecodealliance:main:

The new assembler gives us the opportunity to generate some extra ISLE rules to eliminate boilerplate like the following, hand-written around 180 times throughout inst.isle:

(decl x64_... (Xmm XmmMem) Xmm)
(rule 1 (x64_... src1 src2)
        (if-let true (use_avx))
        (<emit avx version>))
(rule 0 (x64_... src1 src2) (<emit sse version>))

This PR only attempts to solve that exact pattern (i.e., Xmm XmmMem) but the approach could scale to more patterns in the future.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 05 2025 at 22:22):

abrown created PR review comment:

The implementation (below) _could_ all go live in a separate module (?).

view this post on Zulip Wasmtime GitHub notifications bot (Jun 05 2025 at 22:22):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 05 2025 at 22:23):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 05 2025 at 22:23):

abrown created PR review comment:

I'm not terribly happy about dropping this here; it will work fine but at some point we may need to refactor the function this is in. It is just quite handy that all the parameter types and names have been pre-calculated for us.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 05 2025 at 23:04):

cfallin submitted PR review:

Thanks -- neat that this was made to work! Some thoughts below. If this PR turns out to be the best way I'm fine with it going in but I'm curious what you think of the below...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 05 2025 at 23:04):

cfallin created PR review comment:

Can we put this nested match in an opcode() method on rex.opcodes?

Also, it's not totally clear to me what "primary opcode" means in the doc-comment above and why we return the secondary for the "primary" if it exists -- I read the doc-comment for Opcodes below too and I'm still a little fuzzy. Could we say what we mean here by "primary" and why the secondary is the primary?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 05 2025 at 23:04):

cfallin created PR review comment:

It's neat that this was made to work automagically -- all the same, I worry a tiny bit that it may be brittle (unexpectedly determine a match when some subtle condition means they're not quite equivalent; or on the other side, miss a match because of something). The note below about rounding and the fact this limits things to just one format for now as a result seems to bear this out a little.

In the spirit of the explicit DSL, I wonder if it would be better to have a clause on the instruction builder that sets the alternate? Something like

  inst("vpadd", ...).avx_for("padd"),
  inst("padd", ...),
  ...

(avx_for, upgrades, equiv, pick a verb phrase...)

Thoughts?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 06 2025 at 00:05):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 06 2025 at 00:05):

abrown created PR review comment:

Yeah, I did consider adding an explicit version: inst(...).alt("vpadd"). I can't remember what dissuaded me, but probably laziness (there will be a lot of places to update!). But it is more clear so I'm in favor of that — less magic. I'll rework this to use a DSL function and use the current "check the SSE-to-AVX link" logic to verify that the user has specified .alt(...) correctly.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 06 2025 at 00:06):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 06 2025 at 00:06):

cfallin created PR review comment:

Sounds good, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Jun 06 2025 at 00:07):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 06 2025 at 00:07):

abrown created PR review comment:

The manual uses "primary" and "secondary" to explain all this but, yes, it kind of feels like the "primary" opcode should always be the last one. I'll add Opcodes::opcode and try to explain things there a bit better.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 06 2025 at 14:35):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 06 2025 at 14:35):

alexcrichton created PR review comment:

Personally I'd agree with @cfallin as well in that I'm in favor of explicitly saying "this is the AVX thing for that other instruction" which then generates a panic/assertion/etc if something doesn't match. (I realize y'all've already reached this conclusion but wanted to echo my opinion here too to voice support)

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2025 at 16:53):

abrown updated PR #10942.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2025 at 17:48):

alexcrichton submitted PR review:

Nice :+1:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2025 at 18:13):

abrown merged PR #10942.

Last updated: Dec 06 2025 at 06:05 UTC