-
Notifications
You must be signed in to change notification settings - Fork 778
OptimizeInstruction: Optimize any boolean & (No overlap with boolean's LSB) ==> 0 #7505
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.
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
base: main
Are you sure you want to change the base?
Conversation
;; CHECK-NEXT: ) | ||
;; CHECK-NEXT: (i32.const 2) | ||
;; CHECK-NEXT: ) | ||
;; CHECK-NEXT: (i32.const 0) | ||
;; CHECK-NEXT: ) | ||
;; CHECK-NEXT: (drop | ||
;; CHECK-NEXT: (i32.eqz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need new tests for the new function optimizeAndNoOverlappingBits
, including the various corner cases (just one overlapping bit, no overlapping bits, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for teaching, with your suggestions I've found many shortcomings in my PR code. Now, I've updated all.
src/passes/OptimizeInstructions.cpp
Outdated
auto* left = curr->left; | ||
auto* right = curr->right; | ||
|
||
// Check right as constant and left's max bits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment here is in the opposite order of the code. I would reverse it, to follow the code.
auto leftMaxBits = Bits::getMaxBits(left, this); | ||
uint64_t maskLeft; | ||
if (leftMaxBits == 64) { | ||
maskLeft = 0xffffffffffffffffULL; // All bits set for 64-bit case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just return nullptr
in this case? If we know nothing useful about the bits on the left, we can't optimize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can just return nullptr
here.
However, we misses an optimization opportunity for a cornor case. That is, if just return nullptr
, wasm-opt can not optimize
(drop
(i64.and
(i64.const 0x8000000000000000)
(i64.const 0x7fffffffffffffff)
)
)
to
(drop
(i64.const 0)
)
} | ||
} | ||
|
||
// Check left as constant and right's max bits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should not be needed. We canonicalize code so that constants are on the right, in part to help writing optimizations like this.
So you can assume the constant is on the right. (And let me know if you see a case where that is not true - it is a bug.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we removes this part (Check left as constant and right's max bits
), we cannot optimize cases where both left and right are constant.
For example, wouldn't optimize
(i32.and
(i32.const 2)
(i32.const 1)
)
to
(i32.const 0)
The left value i32.const 2
occupies bits [0, 2),
while the constant right is constant 1, overlaps, seems can't optimize.
However, since both values are constants , we can reconsider the perspective:
The right value i32.const 1
occupies bits [0, 1],
and the left constant i32.const 2
is constant 2, thus no overlapped.
Thus, optimization may still be possible.
What do you think? Would this be reasonable, or is there a better way to look at it?
Co-authored-by: Alon Zakai <[email protected]>
There are so many rules for
and
, but we still cannot optimize the following one:to zero.
Adding rule for
add
:any boolean & (No overlap with boolean's LSB) ==> 0
Fixes: #7481