amartosch requested wasmtime-compiler-reviewers for a review on PR #9072.
amartosch requested fitzgen for a review on PR #9072.
amartosch opened PR #9072 from amartosch:cond-traps-in-backend
to bytecodealliance:main
:
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:
- 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.
- 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?
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.
amartosch edited PR #9072:
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:
- 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.
- 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?
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.
fitzgen commented on PR #9072:
Excited to see this come in! Will take a closer look on Monday, thanks!
fitzgen submitted PR review:
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 agrep
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).
fitzgen submitted PR review:
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 agrep
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).
fitzgen created PR review comment:
Can we rename
trap_if_zero
to something liketrap_if_reg
? Having(trap_if_zero $false ...)
mean that we are trapping if something is not zero is a bit too easy to mix up. Even better would be to define a custom enum for zero and not zero because boolean arguments to functions almost always make things harder to read than otherwise.
fitzgen created PR review comment:
Ditto here.
afonso360 commented on PR #9072:
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.
We don't have great support for this in runtests (see https://github.com/bytecodealliance/wasmtime/issues/4781). So we might only be able to test the non trapping path.
afonso360 edited a comment on PR #9072:
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.
We don't have great support for this in runtests (see https://github.com/bytecodealliance/wasmtime/issues/4781). So we might only be able to test the non trapping path.
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 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 2x2 bytes instructions so it ends up not being a big deal.
afonso360 edited a comment on PR #9072:
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.
We don't have great support for this in runtests (see https://github.com/bytecodealliance/wasmtime/issues/4781). So we might only be able to test the non trapping path.
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 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.
amartosch updated PR #9072.
amartosch submitted PR review.
amartosch created PR review comment:
Used enum
amartosch submitted PR review.
amartosch created PR review comment:
Renamed and used an enum
amartosch commented on PR #9072:
Also added very basic runtests that only check non-trapping path for now.
amartosch updated PR #9072.
fitzgen submitted PR review:
Thanks! Looks great!
fitzgen has enabled auto merge for PR #9072.
fitzgen commented on PR #9072:
Strange, for some reason CI hung and this never merged.
fitzgen has disabled auto merge for PR #9072.
fitzgen submitted PR review.
fitzgen created PR review comment:
simple_legalize(&mut self.func, isa);
fitzgen deleted PR review comment.
fitzgen submitted PR review.
fitzgen created PR review comment:
;; Helpers for lowering `trapz` and `trapnz`.
fitzgen updated PR #9072.
fitzgen commented on PR #9072:
Nice, looks like adding a commit triggered CI.
fitzgen has enabled auto merge for PR #9072.
fitzgen edited a comment on PR #9072:
Nice, looks like adding a commit (just rewording a comment) triggered CI.
fitzgen commented on PR #9072:
Sorry for not catching this sooner!
fitzgen updated PR #9072.
fitzgen updated PR #9072.
fitzgen has disabled auto merge for PR #9072.
fitzgen requested wasmtime-core-reviewers for a review on PR #9072.
fitzgen requested alexcrichton for a review on PR #9072.
fitzgen updated PR #9072.
fitzgen updated PR #9072.
fitzgen updated PR #9072.
fitzgen updated PR #9072.
alexcrichton submitted PR review.
alexcrichton merged PR #9072.
Last updated: Dec 23 2024 at 12:05 UTC