Wasmtime GitHub notifications bot (Aug 02 2024 at 21:22): Wasmtime GitHub notifications bot (Aug 02 2024 at 21:22):amartosch requested wasmtime-compiler-reviewers for a review on PR #9072.
Wasmtime GitHub notifications bot (Aug 02 2024 at 21:22):amartosch requested fitzgen for a review on PR #9072.
Wasmtime GitHub notifications bot (Aug 02 2024 at 21:22):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.
Wasmtime GitHub notifications bot (Aug 02 2024 at 21:23):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.
Wasmtime GitHub notifications bot (Aug 02 2024 at 22:19):fitzgen commented on PR #9072:
Excited to see this come in! Will take a closer look on Monday, thanks!
Wasmtime GitHub notifications bot (Aug 06 2024 at 00:49):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.cliffile in https://github.com/bytecodealliance/wasmtime/tree/main/cranelift/filetests/filetests/runtests nor does agrepshow 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).
Wasmtime GitHub notifications bot (Aug 06 2024 at 00:49):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.cliffile in https://github.com/bytecodealliance/wasmtime/tree/main/cranelift/filetests/filetests/runtests nor does agrepshow 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).
Wasmtime GitHub notifications bot (Aug 06 2024 at 00:49):fitzgen created PR review comment:
Can we rename
trap_if_zeroto 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.
Wasmtime GitHub notifications bot (Aug 06 2024 at 00:49):fitzgen created PR review comment:
Ditto here.
Wasmtime GitHub notifications bot (Aug 06 2024 at 08:23):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.
Wasmtime GitHub notifications bot (Aug 06 2024 at 08:29):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.
Wasmtime GitHub notifications bot (Aug 06 2024 at 08:32):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.
Wasmtime GitHub notifications bot (Aug 06 2024 at 18:36):amartosch updated PR #9072.
Wasmtime GitHub notifications bot (Aug 06 2024 at 18:37):amartosch submitted PR review.
Wasmtime GitHub notifications bot (Aug 06 2024 at 18:37):amartosch created PR review comment:
Used enum
Wasmtime GitHub notifications bot (Aug 06 2024 at 18:37):amartosch submitted PR review.
Wasmtime GitHub notifications bot (Aug 06 2024 at 18:37):amartosch created PR review comment:
Renamed and used an enum
Wasmtime GitHub notifications bot (Aug 06 2024 at 18:41):amartosch commented on PR #9072:
Also added very basic runtests that only check non-trapping path for now.
Wasmtime GitHub notifications bot (Aug 06 2024 at 18:49):amartosch updated PR #9072.
Wasmtime GitHub notifications bot (Aug 06 2024 at 20:09):fitzgen submitted PR review:
Thanks! Looks great!
Wasmtime GitHub notifications bot (Aug 06 2024 at 20:09):fitzgen has enabled auto merge for PR #9072.
Wasmtime GitHub notifications bot (Sep 20 2024 at 23:04):fitzgen commented on PR #9072:
Strange, for some reason CI hung and this never merged.
Wasmtime GitHub notifications bot (Sep 20 2024 at 23:04):fitzgen has disabled auto merge for PR #9072.
Wasmtime GitHub notifications bot (Sep 20 2024 at 23:05):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Sep 20 2024 at 23:05):fitzgen created PR review comment:
simple_legalize(&mut self.func, isa);
Wasmtime GitHub notifications bot (Sep 20 2024 at 23:05):fitzgen deleted PR review comment.
Wasmtime GitHub notifications bot (Sep 20 2024 at 23:06):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Sep 20 2024 at 23:06):fitzgen created PR review comment:
;; Helpers for lowering `trapz` and `trapnz`.
Wasmtime GitHub notifications bot (Sep 20 2024 at 23:06):fitzgen updated PR #9072.
Wasmtime GitHub notifications bot (Sep 20 2024 at 23:06):fitzgen commented on PR #9072:
Nice, looks like adding a commit triggered CI.
Wasmtime GitHub notifications bot (Sep 20 2024 at 23:06):fitzgen has enabled auto merge for PR #9072.
Wasmtime GitHub notifications bot (Sep 20 2024 at 23:06):fitzgen edited a comment on PR #9072:
Nice, looks like adding a commit (just rewording a comment) triggered CI.
Wasmtime GitHub notifications bot (Sep 20 2024 at 23:07):fitzgen commented on PR #9072:
Sorry for not catching this sooner!
Wasmtime GitHub notifications bot (Sep 20 2024 at 23:14):fitzgen updated PR #9072.
Wasmtime GitHub notifications bot (Sep 21 2024 at 01:03):fitzgen updated PR #9072.
Wasmtime GitHub notifications bot (Sep 21 2024 at 01:20):fitzgen has disabled auto merge for PR #9072.
Wasmtime GitHub notifications bot (Sep 21 2024 at 01:29):fitzgen requested wasmtime-core-reviewers for a review on PR #9072.
Wasmtime GitHub notifications bot (Sep 21 2024 at 01:29):fitzgen requested alexcrichton for a review on PR #9072.
Wasmtime GitHub notifications bot (Sep 21 2024 at 01:29):fitzgen updated PR #9072.
Wasmtime GitHub notifications bot (Sep 21 2024 at 01:40):fitzgen updated PR #9072.
Wasmtime GitHub notifications bot (Sep 21 2024 at 01:54):fitzgen updated PR #9072.
Wasmtime GitHub notifications bot (Sep 21 2024 at 01:55):fitzgen updated PR #9072.
Wasmtime GitHub notifications bot (Sep 23 2024 at 14:59):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Sep 23 2024 at 15:14):alexcrichton merged PR #9072.
Last updated: Nov 22 2024 at 16:03 UTC