Repository navigation
[Bits] Use abs(rhs) in getMaxBits for rem_s - #9364
Conversation
Move makeSignExt, makePackedFieldGet, getSignExtValue, getAlmostSignExt, getZeroExtValue, Provider::getMaxBits, and Provider::getSignExtKeptBits out of src/ir/bits.h into src/ir/bits.cpp, and add missing direct #includes to files that previously relied on transitive includes from bits.h.
Previously, Provider::getMaxBits passed the signed constant divisor directly to ceilLog2 for RemSInt32 and RemSInt64. For negative divisors (other than INT_MIN), converting to unsigned produced a value with the high bit set, causing ceilLog2 to return 32 or 64 and losing precision even when the dividend was known to be non-negative. Take the unsigned absolute value of the divisor before calling ceilLog2 so negative divisors get the same magnitude bit bound as positive ones.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates Provider::getMaxBits in src/ir/bits.cpp to handle negative divisors by calculating their absolute value as unsigned integers before computing ceilLog2. This enables optimizations like folding redundant masks for negative constant divisors, which is verified by the newly added unit and integration tests. The review feedback points out that applying the unary minus operator to unsigned types (e.g., -uint32_t(rhs)) can trigger MSVC warning C4146, potentially breaking builds that treat warnings as errors. It is recommended to use the suggested 0 - uint32_t(rhs) and 0 - uint64_t(rhs) alternatives to avoid this warning.
# Conflicts: # src/ir/bits.cpp
There was a problem hiding this comment.
Looks like this moves a large amount of code out from a header into source files - can we do that as an NFC PR first? Otherwise, the actual rem_s change is hard to pinpoint.
There was a problem hiding this comment.
Yes, this actually includes an NFC PR that already landed. Merged main to clean up the diff now.
| assert_equal(getMaxBits(&b), 3); | ||
| c0.value = Literal(int32_t(-1)); | ||
| c1.value = Literal(int32_t(-4)); | ||
| assert_equal(getMaxBits(&b), 32); |
There was a problem hiding this comment.
The rhs is -4, which in absolute value is 4, which is 2 bits. Shouldn't the result be 2 and not 32?
There was a problem hiding this comment.
+1 same question. LGTM otherwise
There was a problem hiding this comment.
That comes from here: https://github.com/WebAssembly/binaryen/pull/9364/changes#diff-6ff45ddead31428b20efeba2b73d4e2c4764eb01d31eec2ab168a656e9de78e0R213-R216. It reasons that if the left hand side can be negative, so can the result.
There was a problem hiding this comment.
Hmm, can we remove that pre-existing bit, then? Can leave it to later, maybe with a TODO in the tests that give odd results atm.
There was a problem hiding this comment.
No, that is important for correctness in general. For example, in this test: https://github.com/WebAssembly/binaryen/pull/9364/changes#diff-a94a68e8de6450424e23594987df4a57904920644e71a2f3fb5c481a41ebe1d2R9666-R9676
The only reason that you, the reader, know there is a more precise answer here is because you're looking at a concrete value on the LHS, and not just a judgment of its maximum bits. We could get the more precise result by specifically handling the case where the LHS and RHS are both constants, but that's better left to Precompute.
There was a problem hiding this comment.
Actually, IIUC, -1 %s -4 == -1, so this is already a fully precise result.
There was a problem hiding this comment.
Right, a signed value being divided... I forgot that.
Previously, Provider::getMaxBits passed the signed constant divisor directly to ceilLog2 for RemSInt32 and RemSInt64. For negative divisors (other than INT_MIN), converting to unsigned produced a value with the high bit set, causing ceilLog2 to return 32 or 64 and losing precision even when the dividend was known to be non-negative. Take the unsigned absolute value of the divisor before calling ceilLog2 so negative divisors get the same magnitude bit bound as positive ones.