Add a MachBuffer::defer_trap method

[alexcrichton]

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.

[jameysharp]

This looks like a nice optimization! Overall I think this is a good approach. I just have a couple questions about details and a typo or two.

[jameysharp]

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?

[alexcrichton]

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.

[alexcrichton]

@uweigand If you're able I'd like to get your review on the last commit of this PR, 0fd035c, where I updated how signal handling works on s390x. Trap emission is now buried within MachBuffer so while I could plumb through a flag for "register the trap code on the last byte of the opcode vs the first" I thought it might be easier to update the s390x-specific logic in the signal handler instead to avoid the s390x-specific-ness from spilling over into the mostly-independent MachBuffer. You probably know much more about the intricacies here, though, and I'd like to confirm that things should work as I expect/hope.

[uweigand]

@uweigand If you're able I'd like to get your review on the last commit of this PR, 0fd035c, where I updated how signal handling works on s390x. Trap emission is now buried within MachBuffer so while I could plumb through a flag for "register the trap code on the last byte of the opcode vs the first" I thought it might be easier to update the s390x-specific logic in the signal handler instead to avoid the s390x-specific-ness from spilling over into the mostly-independent MachBuffer. You probably know much more about the intricacies here, though, and I'd like to confirm that things should work as I expect/hope.

Hmm. As far as I can tell, this will work - at least for now. The assumption that only 2-byte instructions can result in a SIGILL is of course not correct in general, that's just a consequence of the particular subset of instructions the back-end is currently using, so I'd be a bit hesitant to hard-code this ...

The whole approach of moving trap instructions to the end of the function may not be the best approach on s390x anyway. Other compilers tend to use a trick (that I haven't implemented yet in cranelift) where the "trap" instruction overlaps the conditional branch instruction. Specifically, to implement a conditional trap, you can use something like

   jgCOND .+2

which is encoded as a 6-byte instruction like so (with M encoding the condition):

  C0 M4 00 00 00 01

If the condition is true, this will branch two bytes forward, landing on the 00 00 bytes embedded in the branch itself - which now double as the trap instruction.

This is the same size as a branch to the end of the function, but doesn't actually require anything to be there.

[alexcrichton]

Ah ok makes sense, would it be best to not apply this change to s390x in that case?

[uweigand]

Ah ok makes sense, would it be best to not apply this change to s390x in that case?

Agreed. Just leave s390x out for now, and I'll implement that other change separately. Thanks!

[jameysharp]

Nice, I enjoyed seeing the filetests become that little bit easier to read. I have one suggestion, but feel free to merge with or without it.

[uweigand]

@alexcrichton I've now implemented the s390x solution discussed above as #6079 .