Skip to content

[Bits] Use abs(rhs) in getMaxBits for rem_s #9364

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tlively merged 6 commits into from
Nov 10, 2026
Merged

[Bits] Use abs(rhs) in getMaxBits for rem_s #9364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
731402a
[NFC] Move larger bits.h functions into bits.cpp
tlively Nov 7, 2026
b1ce7b8
more iwyu
tlively Nov 7, 2026
5edcd93
Merge remote-tracking branch 'origin/main' into bits-cpp
tlively Nov 7, 2026
891b615
[Bits] Use abs(rhs) in getMaxBits for rem_s
tlively Nov 7, 2026
1559efa
Merge remote-tracking branch 'origin/main' into max-bits-rem-s
tlively Nov 9, 2026
36d7778
Merge remote-tracking branch 'origin/main' into max-bits-rem-s
tlively Nov 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/ir/bits.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,9 @@
if (maxBitsLeft == 32) {
return 32;
}
auto bitsRight = Index(ceilLog2(c->value.geti32()));
int32_t rhs = c->value.geti32();
uint32_t absRhs = rhs < 0 ? -uint32_t(rhs) : uint32_t(rhs);

Check warning on line 218 in src/ir/bits.cpp

View workflow job for this annotation

GitHub Actions / coverage

One-sided branch in diff 

Branch `rhs < 0` -> missing FALSE branch (True: 14x, False: 0x)

Check warning on line 218 in src/ir/bits.cpp

View workflow job for this annotation

GitHub Actions / coverage

Uncovered sub-region 

Sub-region `uint32_t(rhs)` on executed line was never reached (0x)
Comment thread
tlively marked this conversation as resolved.
Resolved
auto bitsRight = Index(ceilLog2(absRhs));
return std::min(maxBitsLeft, bitsRight);
}
return 32;
Expand Down Expand Up @@ -310,7 +312,9 @@
if (maxBitsLeft == 64) {
return 64;
}
auto bitsRight = Index(ceilLog2(c->value.geti64()));
int64_t rhs = c->value.geti64();
uint64_t absRhs = rhs < 0 ? -uint64_t(rhs) : uint64_t(rhs);

Check warning on line 316 in src/ir/bits.cpp

View workflow job for this annotation

GitHub Actions / coverage

One-sided branch in diff 

Branch `rhs < 0` -> missing FALSE branch (True: 14x, False: 0x)

Check warning on line 316 in src/ir/bits.cpp

View workflow job for this annotation

GitHub Actions / coverage

Uncovered sub-region 

Sub-region `uint64_t(rhs)` on executed line was never reached (0x)
Comment thread
tlively marked this conversation as resolved.
Resolved
auto bitsRight = Index(ceilLog2(absRhs));
return std::min(maxBitsLeft, bitsRight);
}
return 64;
Expand Down
30 changes: 30 additions & 0 deletions test/example/cpp-unit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,21 @@ void test_bits() {
assert_equal(getMaxBits(&b), 32);
c0.value = Literal(int32_t(3));
c1.value = Literal(int32_t(-1));
assert_equal(getMaxBits(&b), 0);
c0.value = Literal(int32_t(0xFF));
c1.value = Literal(int32_t(-4));
assert_equal(getMaxBits(&b), 2);
c0.value = Literal(int32_t(0xFF));
c1.value = Literal(int32_t(-5));
assert_equal(getMaxBits(&b), 3);
c0.value = Literal(int32_t(-1));
c1.value = Literal(int32_t(-4));
assert_equal(getMaxBits(&b), 32);

@kripken kripken Nov 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rhs is -4, which in absolute value is 4, which is 2 bits. Shouldn't the result be 2 and not 32?

@stevenfontanella stevenfontanella Nov 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 same question. LGTM otherwise

@tlively tlively Nov 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kripken kripken Nov 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tlively tlively Nov 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tlively tlively Nov 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, IIUC, -1 %s -4 == -1, so this is already a fully precise result.

@kripken kripken Nov 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, a signed value being divided... I forgot that.

c0.value = Literal(int32_t(0x7FFFFFFF));
c1.value = Literal(int32_t(0x80000000));
assert_equal(getMaxBits(&b), 31);
c0.value = Literal(int32_t(3));
c1.value = Literal(int32_t(0x80000000));
assert_equal(getMaxBits(&b), 2);
c0.value = Literal(int32_t(0x7FFFFFFF));
c1.value = Literal(int32_t(0x7FFFFFFF));
Expand Down Expand Up @@ -412,6 +427,21 @@ void test_bits() {
assert_equal(getMaxBits(&b), 64);
c0.value = Literal(int64_t(3));
c1.value = Literal(int64_t(-1));
assert_equal(getMaxBits(&b), 0);
c0.value = Literal(int64_t(0xFF));
c1.value = Literal(int64_t(-4));
assert_equal(getMaxBits(&b), 2);
c0.value = Literal(int64_t(0xFF));
c1.value = Literal(int64_t(-5));
assert_equal(getMaxBits(&b), 3);
c0.value = Literal(int64_t(-1));
c1.value = Literal(int64_t(-4));
assert_equal(getMaxBits(&b), 64);
c0.value = Literal(int64_t(0x7FFFFFFFFFFFFFFF));
c1.value = Literal(int64_t(0x8000000000000000));
assert_equal(getMaxBits(&b), 63);
c0.value = Literal(int64_t(3));
c1.value = Literal(int64_t(0x8000000000000000));
assert_equal(getMaxBits(&b), 2);
c0.value = Literal(int64_t(0x7FFFFFFFFFFFFFFF));
c1.value = Literal(int64_t(0x7FFFFFFFFFFFFFFF));
Expand Down
239 changes: 239 additions & 0 deletions test/lit/passes/optimize-instructions-mvp.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9435,6 +9435,245 @@
(i64.const 0x8000000000000000)
))
)
;; CHECK: (func $rem-s-neg-divisor-max-bits-i32 (param $x i32)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.rem_s
;; CHECK-NEXT: (i32.and
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: (i32.const 255)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const -5)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.rem_s
;; CHECK-NEXT: (i32.and
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: (i32.const 255)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const -4)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (local.tee $x
;; CHECK-NEXT: (i32.const 3)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.rem_s
;; CHECK-NEXT: (i32.and
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: (i32.const 2147483647)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const -2147483648)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.and
;; CHECK-NEXT: (i32.rem_s
;; CHECK-NEXT: (i32.and
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: (i32.const 255)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const -5)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const 3)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.and
;; CHECK-NEXT: (i32.rem_s
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: (i32.const -5)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const 7)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $rem-s-neg-divisor-max-bits-i32 (param $x i32)
;; Non-negative dividend with negative constant divisors: getMaxBits uses
;; abs(divisor), allowing redundant masks and zero-bit remainders to fold.
;; abs(-5) = 5 -> max 3 bits (remainder in [0, 4]), so masking with 7 is redundant.
(drop
(i32.and
(i32.rem_s
(i32.and (local.get $x) (i32.const 0xff))
(i32.const -5)
)
(i32.const 7)
)
)
;; abs(-4) = 4 -> max 2 bits (remainder in [0, 3]), so masking with 3 is redundant.
(drop
(i32.and
(i32.rem_s
(i32.and (local.get $x) (i32.const 0xff))
(i32.const -4)
)
(i32.const 3)
)
)
;; Impure non-negative dividend % -1 has 0 max bits and folds to 0 via
;; replaceZeroBitsWithZero while keeping the local.set side effect.
(drop
(i32.rem_s
(local.tee $x (i32.const 3))
(i32.const -1)
)
)
;; Minimum signed i32 divisor (-2147483648) has unsigned abs 2^31 (31 bits).
(drop
(i32.and
(i32.rem_s
(i32.and (local.get $x) (i32.const 0x7fffffff))
(i32.const 0x80000000)
)
(i32.const 0x7fffffff)
)
)
;; Negative case: mask of 3 is too narrow for remainder in [0, 4].
(drop
(i32.and
(i32.rem_s
(i32.and (local.get $x) (i32.const 0xff))
(i32.const -5)
)
(i32.const 3)
)
)
;; Negative case: dividend may be negative, so remainder may be negative and
;; masking with 7 is not redundant.
(drop
(i32.and
(i32.rem_s
(local.get $x)
(i32.const -5)
)
(i32.const 7)
)
)
)
;; CHECK: (func $rem-s-neg-divisor-max-bits-i64 (param $y i64)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i64.rem_s
;; CHECK-NEXT: (i64.and
;; CHECK-NEXT: (local.get $y)
;; CHECK-NEXT: (i64.const 255)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i64.const -5)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i64.rem_s
;; CHECK-NEXT: (i64.and
;; CHECK-NEXT: (local.get $y)
;; CHECK-NEXT: (i64.const 255)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i64.const -4)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result i64)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (local.tee $y
;; CHECK-NEXT: (i64.const 3)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (i64.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i64.rem_s
;; CHECK-NEXT: (i64.and
;; CHECK-NEXT: (local.get $y)
;; CHECK-NEXT: (i64.const 9223372036854775807)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i64.const -9223372036854775808)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i64.and
;; CHECK-NEXT: (i64.rem_s
;; CHECK-NEXT: (i64.and
;; CHECK-NEXT: (local.get $y)
;; CHECK-NEXT: (i64.const 255)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i64.const -5)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i64.const 3)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i64.and
;; CHECK-NEXT: (i64.rem_s
;; CHECK-NEXT: (local.get $y)
;; CHECK-NEXT: (i64.const -5)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i64.const 7)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $rem-s-neg-divisor-max-bits-i64 (param $y i64)
;; Same cases for i64.rem_s.
(drop
(i64.and
(i64.rem_s
(i64.and (local.get $y) (i64.const 0xff))
(i64.const -5)
)
(i64.const 7)
)
)
(drop
(i64.and
(i64.rem_s
(i64.and (local.get $y) (i64.const 0xff))
(i64.const -4)
)
(i64.const 3)
)
)
(drop
(i64.rem_s
(local.tee $y (i64.const 3))
(i64.const -1)
)
)
(drop
(i64.and
(i64.rem_s
(i64.and (local.get $y) (i64.const 0x7fffffffffffffff))
(i64.const 0x8000000000000000)
)
(i64.const 0x7fffffffffffffff)
)
)
;; Negative case: mask of 3 is too narrow for remainder in [0, 4].
(drop
(i64.and
(i64.rem_s
(i64.and (local.get $y) (i64.const 0xff))
(i64.const -5)
)
(i64.const 3)
)
)
;; Negative case: dividend may be negative, so masking with 7 is needed.
(drop
(i64.and
(i64.rem_s
(local.get $y)
(i64.const -5)
)
(i64.const 7)
)
)
)
;; CHECK: (func $srem-by-pot-eq-ne-zero (param $x i32) (param $y i64)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.eqz
Expand Down
Loading