-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a MachBuffer::defer_trap
method
#6011
Add a MachBuffer::defer_trap
method
#6011
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/// label to its offset. The trap will be placed at most `max_distance` | ||
/// from the current offset. | ||
pub fn defer_trap(&mut self, code: TrapCode, stack_map: Option<StackMap>) -> MachLabel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
aa65527
to
0fd035c
Compare
@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 |
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 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
which is encoded as a 6-byte instruction like so (with M encoding the condition):
If the condition is true, this will branch two bytes forward, landing on the This is the same size as a branch to the end of the function, but doesn't actually require anything to be there. |
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! |
67e6ca3
to
1f2491e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
The MachBuffer was registering trap codes with the first byte of the trap, but the SIGILL handler was expecting it to be registered with the last byte of the trap. Exploit that SIGILL is always represented with a 2-byte instruction and always march 2-backwards for SIGILL, continuing to march backwards 1 byte for SIGFPE-generating instructions.
1f2491e
to
38831bf
Compare
Following up on the discussion in bytecodealliance#6011 this adds an improved implementation of TrapIf for s390x using a single conditional branch instruction. If the trap conditions is true, we branch into the middle of the branch instruction - those middle two bytes are zero, which matches the encoding of the trap instruction.
@alexcrichton I've now implemented the s390x solution discussed above as #6079 . |
Following up on the discussion in bytecodealliance#6011 this adds an improved implementation of TrapIf for s390x using a single conditional branch instruction. If the trap conditions is true, we branch into the middle of the branch instruction - those middle two bytes are zero, which matches the encoding of the trap instruction. In addition, show the trap code for Trap and TrapIf instructions in assembler output.
Following up on the discussion in #6011 this adds an improved implementation of TrapIf for s390x using a single conditional branch instruction. If the trap conditions is true, we branch into the middle of the branch instruction - those middle two bytes are zero, which matches the encoding of the trap instruction. In addition, show the trap code for Trap and TrapIf instructions in assembler output.
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 useTrapIf
-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 inMachBuffer
to get patched later on during emission, or at the end of the function. Subsequently allTrapIf
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.