-
Notifications
You must be signed in to change notification settings - Fork 640
spirv-opt: Adding folding rules for bitwise and #6361
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
spirv-opt: Adding folding rules for bitwise and #6361
Conversation
s-perron
left a comment
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.
Sorry, I only saw the commit with the test changes in the my first review, and that is all I thought it did.
I looked at the changes. One of the rules does not seem correct to me.
Also the tests all uses the same bit patterns, and you are not getting full testing. For the add and subtract cases you need to think about cases with a carry, and what happens if there is overflow or underflow.
This change adds the following folding ops: And + Xor/Or: * 0b1110 & (a | 0b0001) = a & 0b1110 * 0b1110 & (a ^ 0b0001) = a & 0b1110 * 0b0110 & (a | 0b1110) = 0b0110 Which *should* resolve KhronosGroup#4902, but since this doesn't add any kind of global variable tracking would be laundered away with any kind of OpPhi. And + Add/Sub: * 1 & (b + 2) = b & 1 * 1 & (b - 2) = b & 1 And + Shifts: * 1 & (b << 1) = 0 * 0x80000000 & (b >> 1) = 0
My logic was based around an add/subtract can only affect bits >= to its lsb, even with overflow/underflow. For example (using an 8bit integer as example): Add Sub |
caaab1c to
bd170e0
Compare
* Renaming ZipIntegerConstants to ForEachIntegerConstantPair * Fixing logical add/sub + and folding rule, where it didn't take into account that masks can have holes in them. * Adding more values in the tests re-running clang-format, after strange formatting
bd170e0 to
0e4b873
Compare
s-perron
left a comment
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.
Looks good, but a few very minor issues.
* Updated description of `ForEachIntegerConstantPair` * Changed nested Op type if, to instead be an early exit * Added `utils::LSB` and replaced `RepeatBitsForward`
Head branch was pushed to by a user without write access
This change adds the following folding ops:
And + Xor/Or
Which should resolve #4902, but since this doesn't add any kind of global variable tracking would be laundered away with any kind of OpPhi.
And + Add/Sub
And + Shifts