Stream: git-wasmtime

Topic: wasmtime / PR #11124 x64: Refactor emission of LoadExtName

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:56):

alexcrichton opened PR #11124 from alexcrichton:x64-cleanup-load-ext-name to bytecodealliance:main:

Don't use raw bytes when emitting this instruction but instead use
symbolic assembler directives to avoid needing to hardcode bytes. This
additionally fixes an issue where with colocated symbols the offset of
the symbol was not taken into account (but for Wasmtime it's always been
0 so this otherwise hasn't come up so far)

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:56):

alexcrichton requested cfallin for a review on PR #11124.

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:56):

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

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:00):

cfallin submitted PR review:

Looks reasonable; thanks! Good catch handling the offsets.

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:00):

cfallin commented on PR #11124:

(The Reg -> Gpr refactoring tripped up in the cranelift-codegen tests, it seems)

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:09):

alexcrichton updated PR #11124.

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:09):

alexcrichton has enabled auto merge for PR #11124.

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:29):

alexcrichton updated PR #11124.

Wasmtime GitHub notifications bot (Jun 24 2025 at 20:44):

alexcrichton updated PR #11124.

Wasmtime GitHub notifications bot (Jun 24 2025 at 20:44):

alexcrichton commented on PR #11124:

Ok to resolve the test failure I had to push up a semi-substantial change as the final commit here. cc @abrown on the assembler change, and mind taking another look @cfallin?

Wasmtime GitHub notifications bot (Jun 24 2025 at 20:55):

cfallin submitted PR review:

New approach looks reasonable, but deferring to @abrown for final opinions on the CodeSink refactor...

Wasmtime GitHub notifications bot (Jun 24 2025 at 20:55):

cfallin created PR review comment:

Maybe a panic here with a panic message including offset? Technically it's unreachable but the unreachability invariant is "remote" (part of another crate) so I'd rather make this easier debug / more verbose if we do miss something later...

Wasmtime GitHub notifications bot (Jun 24 2025 at 20:55):

cfallin created PR review comment:

grammar fix -- "inform ... of a use ... is about to happen" -> "inform ... that a use ... is about to happen"

Wasmtime GitHub notifications bot (Jun 24 2025 at 20:55):

cfallin created PR review comment:

s/the bytes are then placed/the bytes are then expected to be placed/

(it's part of the contract for what the caller must do, rather than a result of this method call, right?)

Wasmtime GitHub notifications bot (Jun 24 2025 at 21:05):

cfallin commented on PR #11124:

(Was this auto-added to merge queue? I gave r+ but Andrew should still weigh in and there were a few comment nits)

Wasmtime GitHub notifications bot (Jun 24 2025 at 21:15):

alexcrichton commented on PR #11124:

Oops I forgot to disable that and pushes from maintainers doesn't auto-disable it, thanks for catching!

Wasmtime GitHub notifications bot (Jun 24 2025 at 23:02):

alexcrichton updated PR #11124.

Wasmtime GitHub notifications bot (Jun 25 2025 at 18:06):

abrown created PR review comment:

Yup, this is better! Thanks for this refactor; I had been worrying a bit about how to get rid of the offset table.

Wasmtime GitHub notifications bot (Jun 25 2025 at 18:06):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jun 25 2025 at 18:06):

abrown created PR review comment:

I guess the decision here is to keep asm::KnownOffset as an index instead of converting it into an enum... I suppose there might be some generic way to pass around a cranelift-codegen-specific KnownOffset type to be a bit safer. But, honestly, that could come much later. This is better already.

Wasmtime GitHub notifications bot (Jun 25 2025 at 18:27):

abrown merged PR #11124.

Wasmtime GitHub notifications bot (Jun 25 2025 at 18:41):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 25 2025 at 18:41):

alexcrichton created PR review comment:

Yeah I was brainstorming how to get a type in there (also for things like ExternalName or relocations) and my conclusion was that we'd have to change enum Amode<R: AsReg> to enum Amode<R: Registers> and then also have:

trait Registers {
    type CodeSink: CodeSink;
    // ...
}

and then store <R::CodeSink as CodeSink>::KnownOffset within an Amode.

That all gets pretty hairy and relatively complicated so I decided to not go down that route quite yet...

Last updated: Dec 06 2025 at 07:03 UTC