Improve morphing of bit comparisons to constant 0/1 #73120

dubiousconst282 · 2022-07-31T06:11:29Z

This PR does the following:

Allow morphing ((x >> CNS(y)) & 1) EQ/NE 0/1 -> (x & CNS(1 << y)) EQ/NE 0 with non-constant y, 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.
Lower EQ(AND(x, 1), 1) into AND(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.
Lower NE/EQ(AND(x, 1), 0/1) into AND(x, 1)

I wasn't able to get the morph heuristic to work with (x >> y & 1) != 1, because the IR looks like EQ(EQ(..., 1), 0), but there are no diffs in this case.

Fixes #72986.

Diffs

    public static string BitTest_If_Ne0(int x, int y) => (x >> y & 1) != 0 ? "X" : " ";
    public 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_If_Ne0(int,int):System.String
-       sarx     eax, ecx, edx
-       test     al, 1
-       jne      SHORT G_M39616_IG05
+       bt       ecx, edx
+       jb       SHORT G_M39616_IG05
        mov      rax, 0xD1FFAB1E      ; " "
        mov      rax, gword ptr [rax]
        ret
        mov      rax, 0xD1FFAB1E      ; "X"
        mov      rax, gword ptr [rax]
        ret
 ; Assembly listing for method Program:BitTest_Eq0(int,int):bool
-       sarx     eax, ecx, edx
-       test     al, 1
-       sete     al
+       bt       ecx, edx
+       setae    al
        movzx    rax, al
        ret
 ; Assembly listing for method Program:BitTest_Eq1(int,int):bool
        sarx     eax, ecx, edx
        and      eax, 1
        ret
 ; Assembly listing for method Program:BitTest_Ne0(int,int):bool
        sarx     eax, ecx, edx
-       test     al, 1
-       setne    al
-       movzx    rax, al
+       and      eax, 1
        ret
 ; Assembly listing for method Program:BitTest_Ne1(int,int):bool
        sarx     eax, ecx, edx
        test     al, 1
        sete     al
        movzx    rax, al
        ret
 ; Assembly listing for method Program:And1_Eq0(int):bool
        xor      eax, eax
        test     cl, 1
        sete     al
        ret
 ; Assembly listing for method Program:And1_Eq1(int):bool
        mov      eax, ecx
        and      eax, 1
        ret
 ; Assembly listing for method Program:And1_Ne0(int):bool
-       xor      eax, eax
-       test     cl, 1
-       setne    al
+       mov      eax, ecx
+       and      eax, 1
        ret
 ; Assembly listing for method Program:And1_Ne1(int):bool
        xor      eax, eax
        test     cl, 1
        sete     al
        ret

SPMI diffs

dnfadmin · 2022-07-31T06:11:43Z

All CLA requirements met.

ghost · 2022-07-31T06:11:45Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR does the following:

Allow morphing ((x >> CNS(y)) & 1) EQ/NE 0/1 -> (x & CNS(1 << y)) EQ/NE 0 with non-constant y. This allows the lowering pass to create BT nodes for the latter pattern.
Lower EQ(AND(x, 1), 1) into AND(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.

Fixes #72986.

Diffs

public 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

Author:	dubiousconst282
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2022-08-02T15:58:16Z

/azp run fuzzlyn

azure-pipelines · 2022-08-02T15:58:46Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS

Changes LGTM. Thanks!

We will hold off on merging until the 7.0 release branches off from main (see #72297 for schedule).

jakobbotsch · 2022-08-02T22:10:02Z

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.

dubiousconst282 · 2022-08-03T04:48:38Z

Thanks! I was holding on draft to investigate regressions from the CI diffs, but there aren't many. I think it's ready now.

AndyAyersMS · 2022-08-26T19:57:31Z

@dubiousconst282 we are ready to take in new changes once more.

Can you fix the merge conflicts?

dubiousconst282 · 2022-08-27T18:35:00Z

@AndyAyersMS Done

AndyAyersMS · 2022-09-01T22:09:42Z

/azp run fuzzlyn

azure-pipelines · 2022-09-01T22:10:03Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2022-09-06T16:04:37Z

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.

OSX arm64 checked

Assert failure(PID 24175 [0x00005e6f], Thread: 2129739 [0x207f4b]): !"Unable to commit a loaderheap code page"
    File: /Users/runner/work/1/s/src/coreclr/utilcode/loaderheap.cpp Line: 1173
    Image: /private/tmp/helix/working/A4B6094A/p/CoreRoot/corerun

AndyAyersMS · 2022-09-06T16:05:20Z

@dubiousconst282 thank you for your contribution!

dubiousconst282 added 3 commits July 31, 2022 01:10

Morph (x >> y & 1) NE/EQ 0/1 into (x & (1 << y)) NE/EQ 0

9e9f6d4

Fix cond reversing

b777d6f

Transform TEST_NE(x, 1) into AND(x, 1)

2994226

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 31, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 31, 2022

dubiousconst282 added 2 commits July 31, 2022 03:50

Recalculate containment when changing TEST to AND

256eb9f

Fix formatting

28985b7

EgorBo added this to the 8.0.0 milestone Jul 31, 2022

dubiousconst282 added 2 commits July 31, 2022 23:46

Update node flags when changing operands

fcf122f

Check heuristic before morphing

8072f4d

JulieLeeMSFT assigned dubiousconst282 and AndyAyersMS Aug 1, 2022

dubiousconst282 added 3 commits August 2, 2022 00:14

Revert TEST_NE -> AND, do it earlier and check for LSH

8504793

Use SetAllEffectsFlags() instead of manual bit ops

f366510

Fix formatting

991ff69

runfoapp bot mentioned this pull request Aug 2, 2022

CI failure: system.security.cryptography.openssl.tests missing libssl #73210

Closed

AndyAyersMS approved these changes Aug 2, 2022

View reviewed changes

Remove unnecessary check

7fef935

dubiousconst282 marked this pull request as ready for review August 3, 2022 04:51

AndyAyersMS added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 26, 2022

Merge branch 'main' into bt-shift-morph

5a0585a

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 27, 2022

dubiousconst282 mentioned this pull request Aug 30, 2022

JIT: Optimize "X & 1 == 0" to "X & 1" #61412

Closed

En3Tho mentioned this pull request Aug 30, 2022

Add more optimizations for (https://github.com/dotnet/runtime/issues/61412) #74806

Merged

AndyAyersMS merged commit 1b4ed22 into dotnet:main Sep 6, 2022

dubiousconst282 deleted the bt-shift-morph branch September 6, 2022 20:24

ghost locked as resolved and limited conversation to collaborators Oct 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve morphing of bit comparisons to constant 0/1 #73120

Improve morphing of bit comparisons to constant 0/1 #73120

dubiousconst282 commented Jul 31, 2022 •

edited by AndyAyersMS

Loading

dnfadmin commented Jul 31, 2022 •

edited

Loading

ghost commented Jul 31, 2022

AndyAyersMS commented Aug 2, 2022

azure-pipelines bot commented Aug 2, 2022

AndyAyersMS left a comment

jakobbotsch commented Aug 2, 2022

dubiousconst282 commented Aug 3, 2022

AndyAyersMS commented Aug 26, 2022

dubiousconst282 commented Aug 27, 2022

AndyAyersMS commented Sep 1, 2022

azure-pipelines bot commented Sep 1, 2022

AndyAyersMS commented Sep 6, 2022

AndyAyersMS commented Sep 6, 2022

Improve morphing of bit comparisons to constant 0/1 #73120

Improve morphing of bit comparisons to constant 0/1 #73120

Conversation

dubiousconst282 commented Jul 31, 2022 • edited by AndyAyersMS Loading

dnfadmin commented Jul 31, 2022 • edited Loading

ghost commented Jul 31, 2022

AndyAyersMS commented Aug 2, 2022

azure-pipelines bot commented Aug 2, 2022

AndyAyersMS left a comment

Choose a reason for hiding this comment

jakobbotsch commented Aug 2, 2022

dubiousconst282 commented Aug 3, 2022

AndyAyersMS commented Aug 26, 2022

dubiousconst282 commented Aug 27, 2022

AndyAyersMS commented Sep 1, 2022

azure-pipelines bot commented Sep 1, 2022

AndyAyersMS commented Sep 6, 2022

AndyAyersMS commented Sep 6, 2022

dubiousconst282 commented Jul 31, 2022 •

edited by AndyAyersMS

Loading

dnfadmin commented Jul 31, 2022 •

edited

Loading