Lower conditional traps in backend #9072
Conversation
|
Excited to see this come in! Will take a closer look on Monday, thanks! |
github-actions
bot
added
cranelift
There was a problem hiding this comment.
Thanks! This is indeed exactly the kind of thing we were imagining in #6055. Overall the PR looks good, just a few nitpicks inline below.
Sequences for s390x. I'm not familiar with the ISA, but conditional trap generation has already been implemented in the backend, so I've just blessed the tests.
This is generally fine. It would be good to double check whether we have run tests and fuzzgen support for conditional traps and add them if they are missing. Then we would be exercising actual execution behavior, which is going to give us a lot more confidence in the correctness of the implementation than blessing output will.
I don't see any trap[n]z.clif file in https://github.com/bytecodealliance/wasmtime/tree/main/cranelift/filetests/filetests/runtests nor does a grep show anything, so it would definitely be good to add as part of this PR.
I think CLIF interpreter and fuzzgen support could happen in follow ups, though.
On RISCV traps are emitted inline; not trapping means jumping over it. On aarch64 and x86_64 they are emitted in islands. I wonder, if there is a deeper reason for this. If not, should the approach be unified with the other backends?
I believe that the answer is that we can implement them inline in risc-v without bloating code size (compared to branching out of line; s390x is inline as well, fwiw) but not on the other architectures. So, for those other architectures, we push the actual trapping instruction out of line to improve icache usage (since traps are extremely rare and effectively terminate the program).
We don't have great support for this in runtests (see #4781). So we might only be able to test the non trapping path.
I attempted to fix this a while ago, but with the conditional branch instruction that we use, we only have an effective jump range of +/-4KiB which is quite restrictive. So we opted to leave the traps inline. This is somewhat mitigated by using the compressed instruction extension, which lets us emit conditional traps using only 2 compressed instructions (4 bytes) so it ends up not being a big deal. |
|
Also added very basic runtests that only check non-trapping path for now. |
|
Strange, for some reason CI hung and this never merged. |
|
Nice, looks like adding a commit (just rewording a comment) triggered CI. |
|
Sorry for not catching this sooner! |
fitzgen
force-pushed
the
cond-traps-in-backend
branch
from
4199ac3
to
62a8701
Compare
Instead of legalizing `trapz` and `trapnz` in the mid-end, we now take them all the way to the backend. This allows us to GVN them and remove redundant trap checks. This also allows us to avoid creating new blocks in the legalizer and otherwise invalidating the control-flow graph.
fitzgen
force-pushed
the
cond-traps-in-backend
branch
from
3bb1841
to
745c604
Compare
fitzgen
force-pushed
the
cond-traps-in-backend
branch
from
745c604
to
e9cb9e6
Compare
fitzgen
force-pushed
the
cond-traps-in-backend
branch
from
e9cb9e6
to
3886384
Compare
This PR moves conditional traps to backends from legalization, as described in #6055. Hope this is what was meant in the issue and is still needed. There are two things I'm not sure about:
This is my first contribution, but I didn't ask any questions because this seemed to be straightforward. Please, feel free to simply discard it if the assumption was wrong.