Wasmtime GitHub notifications bot (Mar 27 2025 at 00:36): Wasmtime GitHub notifications bot (Mar 27 2025 at 00:36):abrown opened PR #10477 from abrown:asm-locks to bytecodealliance:main:
These changes add new instructions that add the
LOCKprefix. TheLOCKprefix is only valid for (1) instruction forms where the destination is a memory operand and (2) instructions in the following set: ADD, ADC, AND, BTC, BTR, BTS, CMPXCHG, CMPXCH8B,
CMPXCHG16B, DEC, INC, NEG, NOT, OR, SBB, SUB, XOR, XADD, and XCHG (not all of which are defined yet). To handle (1) we add a new memory-only operand kind:m{8|16|32|64}.With new
LOCK-prefixed instructions defined, we use them in the ISLE lowering of read-modify-write instructions. The top-levellower.islelowerings underwent some refactoring to handle the fact that these instructions return aSideEffectNoResultbut the lowering rule expects an invalid register to be returned in aReg-to-InstOutputauto conversion.
Wasmtime GitHub notifications bot (Mar 27 2025 at 00:36):abrown requested fitzgen for a review on PR #10477.
Wasmtime GitHub notifications bot (Mar 27 2025 at 00:36):abrown requested wasmtime-compiler-reviewers for a review on PR #10477.
Wasmtime GitHub notifications bot (Mar 27 2025 at 00:38):abrown submitted PR review.
Wasmtime GitHub notifications bot (Mar 27 2025 at 00:38):abrown created PR review comment:
It could be helpful to review this commit separately since it contains all the formatting changes (
cargo fmt) and no functional changes.
Wasmtime GitHub notifications bot (Mar 27 2025 at 00:39):abrown edited PR review comment.
Wasmtime GitHub notifications bot (Mar 27 2025 at 02:14):github-actions[bot] commented on PR #10477:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "cranelift:meta", "isle"Thus the following users have been cc'd because of the following labels:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Mar 27 2025 at 03:25):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 27 2025 at 03:25):alexcrichton created PR review comment:
Question for this: would it would to have
.lock()as a builder on the return value ofinst(..)and that would be an indicator that the lock prefix and non-lock prefix would both be generated? That way we wouldn't have to duplicate the opcodes/feature flags and we could bake-in logic that.lock()requires the first operand to be memory
Wasmtime GitHub notifications bot (Mar 27 2025 at 03:26):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 27 2025 at 03:26):alexcrichton created PR review comment:
To clarify: I'd want the same generated code as what this PR does today, just with less boilerplate on the definitions here by having
.lock()as an agumentation rather than a new set of instructions. It could even internally "expand" somewhere along the way to two instruction values perhaps?
Wasmtime GitHub notifications bot (Mar 27 2025 at 16:53):abrown submitted PR review.
Wasmtime GitHub notifications bot (Mar 27 2025 at 16:53):abrown created PR review comment:
Yeah, I started with that route but abandoned it due to "too much magic." Right now these definitions have the property that each line here shows up as a generated instruction, and either doing that under the hood with
.lock()or alternately setting up aforloop in this file to do the same ends up breaking that simple correspondence. And these new magically-created instructions are actually different enough that, combined with the "new instruction magic," I abandoned that route: they have a new mnemonic (lock_*), they have a new prefix, and--crucially--they only accept memory (not reg-mem) as their destination operand.The other approach I thought through here was to actually model
LOCKas its own separate instruction that would take aBox<LockableInst>or something like that. Intel's manual conceptually treatsLOCKas its own instruction, so it made sense from that perspective. And it would have forced me to create some way to integrate hand-crafted instructions, i.e., instructions that we simply implement by hand but somehow get integrated into the auto-generatedInstenum. But ultimately I decided that, because incranelift-codegen-metawe end up needing new ISLE constructors for anyLOCK-prefixed variants, just creating more boilerplate was the simplest and clearest.
Wasmtime GitHub notifications bot (Mar 28 2025 at 23:48):fitzgen submitted PR review:
LGTM!
Wasmtime GitHub notifications bot (Mar 29 2025 at 00:10):fitzgen merged PR #10477.
Last updated: Apr 17 2025 at 18:04 UTC