-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Improve morphing of bit comparisons to constant 0/1 #73120
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis PR does the following:
Fixes #72986. Diffspublic static bool BitTest_Eq0(int x, int y) => (x >> y & 1) == 0;
public static bool BitTest_Eq1(int x, int y) => (x >> y & 1) == 1;
public static bool BitTest_Ne0(int x, int y) => (x >> y & 1) != 0;
public static bool BitTest_Ne1(int x, int y) => (x >> y & 1) != 1;
public static bool And1_Eq0(int x) => (x & 1) == 0;
public static bool And1_Eq1(int x) => (x & 1) == 1;
public static bool And1_Ne0(int x) => (x & 1) != 0;
public static bool And1_Ne1(int x) => (x & 1) != 1; ; Assembly listing for method Program:BitTest_Eq0(int,int):bool
- 8BC1 mov eax, ecx
- 8BCA mov ecx, edx
- D3F8 sar eax, cl
- A801 test al, 1
- 0F94C0 sete al
+ 0FA3D1 bt ecx, edx
+ 0F93C0 setae al
0FB6C0 movzx rax, al
C3 ret
; Assembly listing for method Program:BitTest_Eq1(int,int):bool
- 8BC1 mov eax, ecx
- 8BCA mov ecx, edx
- D3F8 sar eax, cl
- 83E001 and eax, 1
+ 0FA3D1 bt ecx, edx
+ 0F92C0 setb al
+ 0FB6C0 movzx rax, al
C3 ret
; Assembly listing for method Program:BitTest_Ne0(int,int):bool
- 8BC1 mov eax, ecx
- 8BCA mov ecx, edx
- D3F8 sar eax, cl
- A801 test al, 1
- 0F95C0 setne al
+ 0FA3D1 bt ecx, edx
+ 0F92C0 setb al
0FB6C0 movzx rax, al
C3 ret
; Assembly listing for method Program:BitTest_Ne1(int,int):bool
- 8BC1 mov eax, ecx
- 8BCA mov ecx, edx
- D3F8 sar eax, cl
- A801 test al, 1
- 0F94C0 sete al
+ 0FA3D1 bt ecx, edx
+ 0F93C0 setae al
0FB6C0 movzx rax, al
C3 ret
; Assembly listing for method Program:And1_Eq0(int):bool
33C0 xor eax, eax
F6C101 test cl, 1
0F94C0 sete al
C3 ret
; Assembly listing for method Program:And1_Eq1(int):bool
- 8BC1 mov eax, ecx
- 83E001 and eax, 1
+ B801000000 mov eax, 1
+ 23C1 and eax, ecx
C3 ret
; Assembly listing for method Program:And1_Ne0(int):bool
- 33C0 xor eax, eax
- F6C101 test cl, 1
- 0F95C0 setne al
+ B801000000 mov eax, 1
+ 23C1 and eax, ecx
C3 ret
; Assembly listing for method Program:And1_Ne1(int):bool
33C0 xor eax, eax
F6C101 test cl, 1
0F94C0 sete al
C3 ret
|
/azp run fuzzlyn |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Changes LGTM. Thanks!
We will hold off on merging until the 7.0 release branches off from main
(see #72297 for schedule).
The Fuzzlyn failures look like the ones fixed by #73146. I'm not really sure what happened here, normally jobs triggered via the bot run on a merge of the PR head commit and current main, but in this case it ran directly on the head commit 991ff69 that does not have c4ea81d. I would retrigger it but since this is still a draft I'll wait. |
Thanks! I was holding on draft to investigate regressions from the CI diffs, but there aren't many. I think it's ready now. |
@dubiousconst282 we are ready to take in new changes once more. Can you fix the merge conflicts? |
@AndyAyersMS Done |
/azp run fuzzlyn |
Azure Pipelines successfully started running 1 pipeline(s). |
Interesting and almost certainly unrelated Fuzzlyn failure. No core dump so not sure this is actionable. FYI @janvorli in case we start seeing this more often.
|
@dubiousconst282 thank you for your contribution! |
This PR does the following:
((x >> CNS(y)) & 1) EQ/NE 0/1
->(x & CNS(1 << y)) EQ/NE 0
with non-constanty
, but only in some cases (if it's comparing the result to non-zero, or if the node is being used by a branch). This allows the lowering pass to create BT nodes for the latter pattern.LowerEQ(AND(x, 1), 1)
intoAND(x, 1)
by checking for TEST_NE instead of EQ/NE nodes. This should remove the need to identify variations such as(x & 1) != 0
, while also avoiding interferences with later transforms based on EQ/NE nodes.NE/EQ(AND(x, 1), 0/1)
intoAND(x, 1)
I wasn't able to get the morph heuristic to work with
(x >> y & 1) != 1
, because the IR looks likeEQ(EQ(..., 1), 0)
, but there are no diffs in this case.Fixes #72986.
Diffs
SPMI diffs