Stream: git-wasmtime

Topic: wasmtime / PR #6011 Add a `MachBuffer::defer_trap` method

Wasmtime GitHub notifications bot (Mar 13 2023 at 22:40):

alexcrichton opened PR #6011 from traps-at-end to main:

This commit adds a new method to MachBuffer to defer trap opcodes to the end of a function in a similar manner to how constants are deferred to the end of the function. This is useful for backends which frequently use TrapIf-style opcodes. Currently a jump is emitted which skips the next instruction, a trap, and then execution continues normally. While there isn't any pressing problem with this construction the trap opcode is in the middle of the instruction stream as opposed to "off on the side" despite rarely being taken.

With this method in place all the backends (except riscv64 since I couldn't figure it out easily enough) have a new lowering of their TrapIf opcode. Now a trap is deferred, which returns a label, and then that label is jumped to when executing the trap. A fixup is then recorded in MachBuffer to get patched later on during emission, or at the end of the function. Subsequently all TrapIf instructions translate to a single branch plus a single trap at the end of the function.

I've additionally further updated some more lowerings in the x64 backend which were explicitly using traps to instead use TrapIf where applicable to avoid jumping over traps mid-function. Other backends didn't appear to have many jump-over-the-next-trap patterns.

Lots of tests have had their expectations updated here which should reflect all the traps being sunk to the end of functions.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:54):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:54):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:54):

jameysharp created PR review comment:

This comment is backwards now, isn't it?

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:54):

jameysharp created PR review comment:

How did you decide to put the constant pool before the trap pool? I'm thinking depending on the alignment requirements either order could be best for a given function, so I'm wondering if you had some insight about which was is better or if it just doesn't matter that much.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:54):

jameysharp created PR review comment:

defer_trap doesn't have the max_distance argument that its doc-comment refers to. Do we need a max-distance for branch instructions on any target? Also it isn't "the given label", since it returns the label. I assume the comment was just copied from defer_constant.

As future work, could defer_trap deduplicate the trap pool by returning the same MachLabel when given the same TrapCode? (At least if stack_map is None.) Or do we need to know exactly which check trapped?

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:54):

jameysharp created PR review comment:

Could we print the trap_code in our disassembly, maybe in a comment? It's visible in the Capstone disassembly as part of the trap pool, but it's hard to see which branch goes to which element of the trap pool, and in our disassembly the trap pool isn't printed at all.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:54):

jameysharp created PR review comment:

            // the second condition is true go to the trap.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:54):

jameysharp created PR review comment:

Maybe "next trap-pool opportunity"? Sounds awkward but I haven't come up with better wording.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:00):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:00):

cfallin created PR review comment:

I'd even maybe suggest putting the trap-pool first: it keeps all machine code continuous, which is less surprising both to disassemblers and to humans.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:00):

cfallin edited PR review comment.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:24):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:24):

alexcrichton created PR review comment:

Ah yes indeed no reason for it to be second! I like the idea of putting it first too.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:24):

alexcrichton updated PR #6011 from traps-at-end to main.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:42):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:42):

alexcrichton created PR review comment:

Aha you've reminded me of something I forgot in this PR which was to track the source location of traps to get the correct file origin information when a trap happens. Previously this would naturally happen due to the source location tracking but by moving these to the end the tracking is lost. It turns out we don't have anything in the wasm test suite testing this just yet! I've now added in source location tracking for deferred traps in addition to some more tests which require the logic to be here.

So now to get back to your question, about deduplicating, maybe! The source location adds another vector by which this needs to be considered for deduplication, and it's pretty unlikely to get deduplicated with the source information (except for perhaps some of those x64 lowerings of float-to-int conversions which have a bunch of traps). With source locations considered too, though, I think it's safe to deduplicate in the future.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:43):

alexcrichton updated PR #6011 from traps-at-end to main.

Wasmtime GitHub notifications bot (Mar 14 2023 at 03:52):

alexcrichton updated PR #6011 from traps-at-end to main.

Wasmtime GitHub notifications bot (Mar 14 2023 at 03:56):

alexcrichton updated PR #6011 from traps-at-end to main.

Wasmtime GitHub notifications bot (Mar 14 2023 at 18:42):

alexcrichton updated PR #6011 from traps-at-end to main.

Wasmtime GitHub notifications bot (Mar 14 2023 at 18:59):

alexcrichton updated PR #6011 from traps-at-end to main.

Wasmtime GitHub notifications bot (Mar 16 2023 at 18:59):

alexcrichton updated PR #6011 from traps-at-end to main.

Wasmtime GitHub notifications bot (Mar 17 2023 at 14:58):

alexcrichton requested jameysharp for a review on PR #6011.

Wasmtime GitHub notifications bot (Mar 20 2023 at 19:15):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 20 2023 at 19:15):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 20 2023 at 19:15):

jameysharp created PR review comment:

What would you think of this instead of checking for string equality below?

    for (ty, min) in [("i32", i32::MIN as u64), ("i64", i64::MIN as u64)] {

Wasmtime GitHub notifications bot (Mar 20 2023 at 20:46):

alexcrichton updated PR #6011 from traps-at-end to main.

Wasmtime GitHub notifications bot (Mar 20 2023 at 20:56):

alexcrichton has enabled auto merge for PR #6011.

Wasmtime GitHub notifications bot (Mar 20 2023 at 21:56):

alexcrichton merged PR #6011.

Last updated: Nov 22 2024 at 17:03 UTC