Wasmtime GitHub notifications bot (Mar 15 2023 at 17:44): Wasmtime GitHub notifications bot (Mar 15 2023 at 17:44):alexcrichton opened PR #6026 from x64-sigfpe to main:
Prior to this commit Wasmtime would configure
avoid_div_traps=trueunconditionally for Cranelift. This, for the division-based instructions, would change emitted code to explicitly trap on trap conditions instead of letting thedivx86 instruction trap.There's no specific reason for Wasmtime, however, to specifically avoid traps in the
divinstruction. This means that the extra generated branches on x86 aren't necessary since thedivandidivinstructions already trap for similar conditions as wasm requires.This commit instead disables the
avoid_div_trapssetting for Wasmtime's usage of Cranelift. Subsequently the codegen rules were updated slightly:
- When
avoid_div_traps=true, traps are no longer emitted fordivinstructions.- The
udiv/ureminstructions now list their trap as divide-by-zero instead of integer overflow.- The lowering for
sdivwas updated to still explicitly check for zero but the integer overflow case is deferred to the instruction itself.- The lowering of
sremno longer checks for zero and the listed trap for thedivinstruction is a divide-by-zero.This means that the codegen for
udivanduremno longer have any branches. The codegen forsdivremoves one branch but keeps the zero-check to differentiate the two kinds of traps. The codegen forsremremoves one branch but keeps the -1 check since the semantics ofsremmismatch with the semantics ofidivwith a -1 divisor (specifically for INT_MIN).This is unlikely to have really all that much of a speedup but was something I noticed during #6008 which seemed like it'd be good to clean up. Plus Wasmtime's signal handling was already set up to catch
SIGFPE, it was just never firing.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Mar 15 2023 at 18:52):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Mar 15 2023 at 18:52):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Mar 15 2023 at 18:52):cfallin created PR review comment:
I wonder if it would be simpler to include a
trap_codefield in the instruction, and set this when creating it in the lowering? That would avoid the implicit state flow via the flag on theEmitStateand would also place this conditional logic closer to where the rest of the instruction sequence is specified.
Wasmtime GitHub notifications bot (Mar 15 2023 at 22:16):alexcrichton updated PR #6026 from x64-sigfpe to main.
Wasmtime GitHub notifications bot (Mar 15 2023 at 22:18):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 15 2023 at 22:18):alexcrichton created PR review comment:
Ah good suggestion!
Wasmtime GitHub notifications bot (Mar 15 2023 at 22:44):alexcrichton updated PR #6026 from x64-sigfpe to main.
Wasmtime GitHub notifications bot (Mar 15 2023 at 22:50):alexcrichton updated PR #6026 from x64-sigfpe to main.
Wasmtime GitHub notifications bot (Mar 15 2023 at 23:40):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Mar 15 2023 at 23:45):alexcrichton updated PR #6026 from x64-sigfpe to main.
Wasmtime GitHub notifications bot (Mar 15 2023 at 23:59):alexcrichton has enabled auto merge for PR #6026.
Wasmtime GitHub notifications bot (Mar 16 2023 at 00:59):alexcrichton merged PR #6026.
Last updated: Dec 23 2024 at 12:05 UTC