pulley: Implement a new br_table instruction
#9659
Conversation
alexcrichton
requested review from
abrown and
pchickey
and removed request for
a team
github-actions
bot
added
cranelift
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "isle", "pulley"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
alexcrichton
force-pushed
the
pulley-br-table
branch
from
950fd0b
to
708fa5d
Compare
This is intended to match WebAssembly's `br_table` and Cranelift's version as well. This is implemented as a new `br_table32` opcode where a 32-bit number of branch targets are encoded after `br_table32` all as a `PcRelOffset`, 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.
alexcrichton
force-pushed
the
pulley-br-table
branch
from
708fa5d
to
91b836c
Compare
| if sink.island_needed(br_table_size) { | ||
| let label = sink.get_label(); | ||
| <InstAndKind<P>>::from(Inst::Jump { label }).emit(sink, emit_info, state); | ||
| sink.emit_island(br_table_size, &mut state.ctrl_plane); | ||
| sink.bind_label(label, &mut state.ctrl_plane); | ||
| } |
There was a problem hiding this 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 at pc, etc. We don't have to jump an arbitrary u32 amount. Or is there something that I don't understand here about the tail-calling version of Pulley?
There was a problem hiding this 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 pc like native code and we'll probably add smaller-than-32-bit jumps at some point.
| } else { | ||
| write!(result, " ")?; | ||
| for (i, line) in disas.lines().enumerate() { | ||
| if i == 0 && (write_offsets || (prev_jump && !is_jump)) { |
There was a problem hiding this 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?
There was a problem hiding this 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.
| @@ -104,8 +104,8 @@ where | |||
| // (with an `EmitIsland`). We check this in debug builds. This is `mut` | |||
| // to allow disabling the check for `JTSequence`, which is always | |||
| // emitted following an `EmitIsland`. | |||
| let start = sink.cur_offset(); | |||
| pulley_emit(self, sink, emit_info, state, start); | |||
| let mut start = sink.cur_offset(); | |||
There was a problem hiding this 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).
There was a problem hiding this comment.
FWIW this is modeled after the aarch64 backend
Co-authored-by: Andrew Brown <[email protected]>
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.