Add optimization from x < 0 || x > POSITIVE_CONST to unsigned(x) > POSITIVE_CONST
#6999
Conversation
…CONST` with unsigned comparison Fixes WebAssembly#6685
There was a problem hiding this comment.
As you said, in general we have been thinking to leave this kind of thing for LLVM. But I am also happy with landing such PRs if they are well-tested (including fuzzing) and well-written, and this PR is pretty close, see the comments, so I think it makes sense to move forward with this.
src/passes/OptimizeInstructions.cpp
Outdated
| // x < 0 || x > POSITIVE_CONST ==> x > POSITIVE_CONST (unsigned | ||
| // comparison) |
There was a problem hiding this comment.
| // x < 0 || x > POSITIVE_CONST ==> x > POSITIVE_CONST (unsigned | |
| // comparison) | |
| // x < 0 || x > POSITIVE_CONST ==> unsigned(x) > POSITIVE_CONST |
(this is the convention we have elsewhere in the file to indicate an unsigned comparison)
src/passes/OptimizeInstructions.cpp
Outdated
| // x < 0 || x > POSITIVE_CONST ==> x > POSITIVE_CONST (unsigned | ||
| // comparison) | ||
| Binary* bin; | ||
| Expression *x_left, *x_right; |
There was a problem hiding this comment.
| Expression *x_left, *x_right; | |
| Expression *xLeft, *xRight; |
src/passes/OptimizeInstructions.cpp
Outdated
| binary(LtSInt32, pure(&x_left), i32(0)), | ||
| binary(GtSInt32, pure(&x_right), constant(&y)))) && | ||
| y->type == Type::i32 && y->value.geti32() > 0 && | ||
| ExpressionAnalyzer::equal(x_left, x_right)) { |
There was a problem hiding this comment.
Rather than using pure + ::equal, see areConsecutiveInputsEqualAndFoldable (they are consecutive aside from constants (which are not an issue), we need them to be equal, and we want to fold them together).
|
Thanks for the feedback! I'll check the fuzzing tests as well |
|
@kripken not sure, but looks like a bug in the fuzzer (as stated in the message by fuzzer :D). Fuzzer output:Test file:(module
(global $g (mut i32) (i32.const 0))
(func $cmp (param $0 i32) (result i32)
(i32.or
(i32.lt_s
(local.get $0)
(i32.const 0)
)
(i32.gt_s
(local.get $0)
(i32.const 255)
)
)
)
(func $cmp2 (param $0 i32) (result i32)
(i32.or
(i32.lt_s
(local.get $0)
(i32.const 0)
)
(i32.gt_s
(local.get $0)
(i32.const -255)
)
)
)
(func $cmp3 (param $0 i32) (result i32)
(i32.or
(i32.lt_s
(local.get $0)
(i32.const 10)
)
(i32.gt_s
(local.get $0)
(i32.const 255)
)
)
)
(func $set_global_and_return (param $x i32) (result i32)
(global.set $g (local.get $x))
(local.get $x)
)
(func $cmp4 (result i32)
(i32.or
(i32.lt_s
(call $set_global_and_return (i32.const 10))
(i32.const 0)
)
(i32.gt_s
(call $set_global_and_return (i32.const 10))
(i32.const 255)
)
)
)
(func $cmp5 (param $0 i32) (param $1 i32) (result i32)
(i32.or
(i32.lt_s
(local.get $0)
(i32.const 0)
)
(i32.gt_s
(local.get $1)
(i32.const 255)
)
)
)
(func $cmp6 (param $0 i32) (param $1 i32) (result i32)
(i32.or
(i32.lt_s
(local.get $0)
(i32.const 0)
)
(i32.gt_s
(local.get $0)
(local.get $1)
)
)
)
)Python 3.12.6 (main, Sep 6 2024, 19:03:47) [Clang 15.0.0 (clang-1500.1.0.2.5)] on darwin |
|
But most likely the problem is on my side, since even |
i582
changed the title
x < 0 || x > POSITIVE_CONST to x > POSITIVE_CONST with unsigned comparisonx < 0 || x > POSITIVE_CONST to unsigned(x) > POSITIVE_CONST
|
Strange about that fuzzer error. You might need to debug that line |
| OrInt32, | ||
| binary(LtSInt32, any(&xLeft), i32(0)), | ||
| binary(GtSInt32, any(&xRight), constant(&y)))) && | ||
| y->type == Type::i32 && y->value.geti32() > 0 && |
There was a problem hiding this comment.
| y->type == Type::i32 && y->value.geti32() > 0 && | |
| y->value.geti32() > 0 && |
The type must be i32 because the operation is GtSInt32.
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
| (func $cmp (param $0 i32) (result i32) | ||
| (i32.or |
There was a problem hiding this comment.
Please add comments in the tests, like here, "we can optimize this to a single i32.gt_u".
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
| (func $cmp2 (param $0 i32) (result i32) | ||
| (i32.or |
There was a problem hiding this comment.
The comment on this one could be "we cannot optimize here because the second constant is negative".
| (i32.gt_s | ||
| (local.get $0) | ||
| (i32.const -255) ;; negative number | ||
| ) |
There was a problem hiding this comment.
Please add a testcase where both constants are 0.
Hey, I read the pinned issue carefully and probably this optimization is not really needed with LLVM, but I find this task simple enough to learn more about binaryen. If you think this PR is completely useless, I understand, anyway I learned a lot about binaryen while doing it :)
I also need help, how do I compare two expressions for equivalence?Fixes #6685