Repository navigation
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.
There was a problem hiding this comment.
Code Review
This pull request introduces a new src/ir/bits.cpp file to centralize bit manipulation utilities and updates various headers and build configurations to support this module. The review highlighted a missing direct include for ir/properties.h and identified precision loss issues in the bit-tracking logic for RemSInt32 and RemSInt64 when dealing with negative divisors, providing actionable code suggestions to resolve these bugs.
| */ | ||
|
|
||
| #include "ir/bits.h" | ||
| #include "ir/boolean.h" |
There was a problem hiding this comment.
The header ir/boolean.h is included but not directly used in this file. Instead, Properties::emitsBoolean is used, which is defined in ir/properties.h. To adhere to the principle of direct includes and avoid relying on transitive includes, ir/properties.h should be included directly.
| #include "ir/boolean.h" | |
| #include "ir/properties.h" |
| case RemSInt32: { | ||
| if (auto* c = binary->right->dynCast<Const>()) { | ||
| auto maxBitsLeft = getMaxBits(binary->left); | ||
| // if left may be negative, the result may be negative | ||
| if (maxBitsLeft == 32) { | ||
| return 32; | ||
| } | ||
| auto bitsRight = Index(ceilLog2(c->value.geti32())); | ||
| return std::min(maxBitsLeft, bitsRight); | ||
| } |
There was a problem hiding this comment.
When the divisor c is negative, c->value.geti32() returns a negative value. Passing a negative value to ceilLog2 (which expects an unsigned value) causes it to be cast to a very large unsigned integer, resulting in ceilLog2 returning 32 and losing precision in the bit-tracking analysis. Using the absolute value of the divisor avoids this precision loss.
case RemSInt32: {
if (auto* c = binary->right->dynCast<Const>()) {
auto maxBitsLeft = getMaxBits(binary->left);
// if left may be negative, the result may be negative
if (maxBitsLeft == 32) {
return 32;
}
auto val = c->value.geti32();
auto absVal = val < 0 ? -uint32_t(val) : uint32_t(val);
auto bitsRight = Index(ceilLog2(absVal));
return std::min(maxBitsLeft, bitsRight);
}| case RemSInt64: { | ||
| if (auto* c = binary->right->dynCast<Const>()) { | ||
| auto maxBitsLeft = getMaxBits(binary->left); | ||
| // if left may be negative, the result may be negative | ||
| if (maxBitsLeft == 64) { | ||
| return 64; | ||
| } | ||
| auto bitsRight = Index(ceilLog2(c->value.geti64())); | ||
| return std::min(maxBitsLeft, bitsRight); | ||
| } |
There was a problem hiding this comment.
Similar to RemSInt32, when the divisor c is negative, c->value.geti64() returns a negative value. Passing this to ceilLog2 causes a cast to a very large unsigned integer, resulting in ceilLog2 returning 64 and losing precision. Using the absolute value of the divisor avoids this precision loss.
case RemSInt64: {
if (auto* c = binary->right->dynCast<Const>()) {
auto maxBitsLeft = getMaxBits(binary->left);
// if left may be negative, the result may be negative
if (maxBitsLeft == 64) {
return 64;
}
auto val = c->value.geti64();
auto absVal = val < 0 ? -uint64_t(val) : uint64_t(val);
auto bitsRight = Index(ceilLog2(absVal));
return std::min(maxBitsLeft, bitsRight);
}
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.