Repository navigation
Conversation
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.
| } | ||
| auto bitsRight = Index(ceilLog2(c->value.geti32())); | ||
| int32_t rhs = c->value.geti32(); | ||
| uint32_t absRhs = rhs < 0 ? -uint32_t(rhs) : uint32_t(rhs); |
There was a problem hiding this comment.
Applying the unary minus operator to an unsigned type (e.g., -uint32_t(rhs)) can trigger compiler warnings on MSVC (specifically warning C4146: 'unary minus operator applied to unsigned type, result still unsigned'). Since this codebase is compiled on multiple platforms including Windows with MSVC, and warnings are often treated as errors, this can break the build. Using 0 - uint32_t(rhs) is a safe, warning-free alternative that achieves the same result.
| uint32_t absRhs = rhs < 0 ? -uint32_t(rhs) : uint32_t(rhs); | |
| uint32_t absRhs = rhs < 0 ? 0 - uint32_t(rhs) : uint32_t(rhs); |
| } | ||
| auto bitsRight = Index(ceilLog2(c->value.geti64())); | ||
| int64_t rhs = c->value.geti64(); | ||
| uint64_t absRhs = rhs < 0 ? -uint64_t(rhs) : uint64_t(rhs); |
There was a problem hiding this comment.
Applying the unary minus operator to an unsigned type (e.g., -uint64_t(rhs)) can trigger compiler warnings on MSVC (specifically warning C4146: 'unary minus operator applied to unsigned type, result still unsigned'). Since this codebase is compiled on multiple platforms including Windows with MSVC, and warnings are often treated as errors, this can break the build. Using 0 - uint64_t(rhs) is a safe, warning-free alternative that achieves the same result.
| uint64_t absRhs = rhs < 0 ? -uint64_t(rhs) : uint64_t(rhs); | |
| uint64_t absRhs = rhs < 0 ? 0 - uint64_t(rhs) : uint64_t(rhs); |
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.