Wasmtime GitHub notifications bot (Nov 22 2024 at 20:39): Wasmtime GitHub notifications bot (Nov 22 2024 at 20:39):alexcrichton opened PR #9659 from alexcrichton:pulley-br-table to bytecodealliance:main:
This is intended to match WebAssembly's
br_tableand Cranelift's version as well. This is implemented as a newbr_table32opcode where a 32-bit number of branch targets are encoded afterbr_table32all as aPcRelOffset, a 32-bit offset. This helps bake in a more "macro opcode" into the interpreter rather than a handful of more primitive opcodes that would achieve the same result with loads/indirect jumps/comparisons/etc.<!--
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.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Nov 22 2024 at 20:39):alexcrichton requested abrown for a review on PR #9659.
Wasmtime GitHub notifications bot (Nov 22 2024 at 20:39):alexcrichton requested wasmtime-compiler-reviewers for a review on PR #9659.
Wasmtime GitHub notifications bot (Nov 22 2024 at 20:39):alexcrichton requested pchickey for a review on PR #9659.
Wasmtime GitHub notifications bot (Nov 22 2024 at 20:39):alexcrichton requested wasmtime-core-reviewers for a review on PR #9659.
Wasmtime GitHub notifications bot (Nov 22 2024 at 20:39):alexcrichton requested wasmtime-default-reviewers for a review on PR #9659.
Wasmtime GitHub notifications bot (Nov 22 2024 at 20:42):alexcrichton updated PR #9659.
Wasmtime GitHub notifications bot (Nov 22 2024 at 21:44):github-actions[bot] commented on PR #9659:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This issue or pull request has been labeled: "cranelift", "isle", "pulley"Thus the following users have been cc'd because of the following labels:
- cfallin: isle
- fitzgen: isle, pulley
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 (Nov 22 2024 at 22:58):alexcrichton updated PR #9659.
Wasmtime GitHub notifications bot (Nov 22 2024 at 23:01):alexcrichton updated PR #9659.
Wasmtime GitHub notifications bot (Nov 22 2024 at 23:10):alexcrichton updated PR #9659.
Wasmtime GitHub notifications bot (Nov 25 2024 at 20:31):abrown submitted PR review.
Wasmtime GitHub notifications bot (Nov 25 2024 at 20:31):abrown created PR review comment:
This is confusing: if Pulley is an interpreter, we're not going to need an island for the same reasons we did during JIT compilation. I haven't looked below, but typically an interpreter sets the
pc, the dispatch loop loads the bytecode atpc, etc. We don't have to jump an arbitraryu32amount. Or is there something that I don't understand here about the tail-calling version of Pulley?
Wasmtime GitHub notifications bot (Nov 25 2024 at 20:31):abrown created PR review comment:
It seems like we're trying to only have the targets of jumps be prefixed by address labels but then the test output ends up printing labels for everything after
0x33?
Wasmtime GitHub notifications bot (Nov 25 2024 at 20:31):abrown created PR review comment:
;; jumps to `default` if it's out-of-bounds.
Wasmtime GitHub notifications bot (Nov 25 2024 at 20:31):abrown created PR review comment:
Not having worked on Pulley from the beginning, I'm stepping out on a limb here: it's worrying that once we introduce mutable state during codegen (and later during disassembly) we will have difficulty going back from this. This feels like a better discussion to have with @fitzgen since he may have stronger opinions about this (or not).
Wasmtime GitHub notifications bot (Nov 25 2024 at 22:43):alexcrichton updated PR #9659.
Wasmtime GitHub notifications bot (Nov 25 2024 at 22:48):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 25 2024 at 22:48):alexcrichton created PR review comment:
Heh your confusion is not unwarranted! Pulley is roughly modeled after our existing backends which is why this is still a problem. For example a jump will be "jump this distance forward or backward" in the bytecode stream (e.g. relative offset to pc). The jumps may have limited distance like other backends (right now they're all 32-bit jumps though) which is why islands might be necessary. Basically Pulley still has the same issues as other backends in terms of handling jumps b/c it's all relative to the current
pclike native code and we'll probably add smaller-than-32-bit jumps at some point.
Wasmtime GitHub notifications bot (Nov 25 2024 at 22:49):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 25 2024 at 22:49):alexcrichton created PR review comment:
Yeah the idea is that only the initial line has an offset and the others don't for multi-line instructions. The behavior in the test has to do with how the test disassembly tries to omit instruction offsets but then includes instruction offsets if they might be targets of a jump (so it's known where a jump is going). The logic is relatively primitive though and falls down quickly, so the all-offsets-after-0x33 isn't a result of this logic but is something preexisting.
Wasmtime GitHub notifications bot (Nov 25 2024 at 22:50):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 25 2024 at 22:50):alexcrichton created PR review comment:
FWIW this is modeled after the aarch64 backend
Wasmtime GitHub notifications bot (Dec 02 2024 at 18:51):abrown submitted PR review.
Wasmtime GitHub notifications bot (Dec 02 2024 at 18:51):abrown created PR review comment:
Ok, if you're saying "we need islands for programs with more than 4GB of Pulley bytecode" I think I see what you mean. But... it also doesn't seem unreasonable to disallow that and avoid the island complexity. You're in the middle of this so you have a better handle on this; leaving it up to you!
Wasmtime GitHub notifications bot (Dec 02 2024 at 18:52):abrown submitted PR review:
I don't want to stand in the way of progress: Pulley is getting better!
Wasmtime GitHub notifications bot (Dec 02 2024 at 19:00):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 02 2024 at 19:01):alexcrichton created PR review comment:
For now yeah that's definitely the constraint, in the future though I think we might add 8-bit or smaller jump targets in which case we'd definitely want the island bits. So in some sense this is future-proofing if not maintaining consistency across the backends.
Wasmtime GitHub notifications bot (Dec 02 2024 at 19:17):alexcrichton merged PR #9659.
Last updated: Dec 23 2024 at 12:05 UTC