Skip to content
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

Optimize u>=1 to u!=0 and u<1 to u==0 #25458

Merged
merged 6 commits into from Oct 16, 2019

Conversation

@EgorBo
Copy link

@EgorBo EgorBo commented Jun 27, 2019

JIT already applies these tricks for signed types. This PR adds support for unsigned (extends existing logic). The diff looks big because I removed the surrounding if-else block

using System.Runtime.CompilerServices;

class MyTests
{
    // make sure my changes don't regress signed types:
    bool C1(int a) => a >= 1;   // to "a > 0"
    bool C2(int a) => a < 1;    // to "a <= 0"
    bool C3(int a) => a <= -1;  // to "a < 0"
    bool C4(int a) => a > -1;   // to "a >= 0"

    // new cases:
    bool UC1(uint a) => a >= 1; // to "a != 0"
    bool UC2(uint a) => a < 1;  // to "a == 0"
}
; Method MyTests:C1(int):bool:this
G_M54558_IG02:
       test     edx, edx
       setg     al
       movzx    rax, al
; Total bytes of code: 9


; Method MyTests:C2(int):bool:this
G_M54557_IG02:
       test     edx, edx
       setle    al
       movzx    rax, al
; Total bytes of code: 9


; Method MyTests:C3(int):bool:this
G_M54556_IG02:
       test     edx, edx
       setl     al                  ; LLVM generates shr eax, 31
       movzx    rax, al
; Total bytes of code: 9


; Method MyTests:C4(int):bool:this
G_M54555_IG02:
       test     edx, edx
       setge    al
       movzx    rax, al
; Total bytes of code: 9


; Method MyTests:UC1(int):bool:this
G_M19883_IG02:
-      cmp      edx, 1
-      setae    al
+      test     edx, edx
+      setne    al
       movzx    rax, al
-; Total bytes of code: 10
+; Total bytes of code: 9


; Method MyTests:UC2(int):bool:this
G_M19880_IG02:
-      cmp      edx, 1
-      setb     al
+      test     edx, edx
+      sete     al
       movzx    rax, al
-; Total bytes of code: 10
+; Total bytes of code: 9

/cc: @mikedn

src/jit/morph.cpp Outdated Show resolved Hide resolved
Loading
src/jit/morph.cpp Outdated Show resolved Hide resolved
Loading
src/jit/morph.cpp Outdated Show resolved Hide resolved
Loading
src/jit/morph.cpp Outdated Show resolved Hide resolved
Loading
@mikedn
Copy link

@mikedn mikedn commented Jun 28, 2019

Any diffs?

Loading

@EgorBo
Copy link
Author

@EgorBo EgorBo commented Jun 28, 2019

@mikedn is there a doc on how to do it properly for the whole coreclr/corefx? I am only using COMPlus_JitDisasm. And my own add-in for VS for simple diffs 🙂:

fgege

Loading

@mikedn
Copy link

@mikedn mikedn commented Jun 28, 2019

For diffs you need jit-diff from the jitutils repository: https://github.com/dotnet/jitutils/blob/master/doc/diffs.md

Running jit-diff itself shouldn't be too difficult but you need a coreclr "baseline". The docs recommend using a separate coreclr clone but I find that to be overkill. I normally just build master and save the clrjit.dll in a "base" folder and pass that to jit-diff.

Loading

@mikedn
Copy link

@mikedn mikedn commented Jun 28, 2019

Here's the x64 FX diff:

Found 3 files with textual diffs.
Crossgen Diffs for System.Private.CoreLib.dll, framework assemblies for x64 default jit
Summary:
(Lower is better)
Total bytes of diff: -280 (-0.00% of base)
    diff is an improvement.
Top file improvements by size (bytes):
        -272 : System.Private.CoreLib.dasm (-0.01% of base)
          -8 : System.Private.Xml.dasm (-0.00% of base)
2 total files with size differences (2 improved, 0 regressed), 127 unchanged.
Top method improvements by size (bytes):
        -198 (-4.88% of base) : System.Private.CoreLib.dasm - Vector64`1:ToString():ref:this (11 methods)
         -52 (-3.44% of base) : System.Private.CoreLib.dasm - Vector64`1:Equals(struct):bool:this (11 methods)
         -10 (-11.36% of base) : System.Private.CoreLib.dasm - Vector64:GetElement(struct,int):long (2 methods)
          -7 (-0.92% of base) : System.Private.Xml.dasm - BigNumber:Mul(byref):this
          -6 (-0.61% of base) : System.Private.CoreLib.dasm - Vector64`1:GetHashCode():int:this (11 methods)
Top method improvements by size (percentage):
         -10 (-11.36% of base) : System.Private.CoreLib.dasm - Vector64:GetElement(struct,int):long (2 methods)
          -5 (-11.11% of base) : System.Private.CoreLib.dasm - Vector64:GetElement(struct,int):double
        -198 (-4.88% of base) : System.Private.CoreLib.dasm - Vector64`1:ToString():ref:this (11 methods)
         -52 (-3.44% of base) : System.Private.CoreLib.dasm - Vector64`1:Equals(struct):bool:this (11 methods)
          -7 (-0.92% of base) : System.Private.Xml.dasm - BigNumber:Mul(byref):this
8 total methods with size differences (8 improved, 0 regressed), 146770 unchanged.
1 files had text diffs but not size diffs.
Microsoft.CodeAnalysis.VisualBasic.dasm had 12 diffs

Loading

// will still compute the same value as before
tree->SetOper(oper, GenTree::PRESERVE_VN);
cns2->gtIntCon.gtIconVal = 0;
SET_OPER:
Copy link

@ArtBlnd ArtBlnd Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better replace it to outside of else if (!tree->IsUnsigned() && cns2->IsIntegralConst(-1)) scope and also its end of if (op2->gtOper == GT_CNS_INT)?

Loading

@BruceForstall
Copy link
Member

@BruceForstall BruceForstall commented Aug 3, 2019

Loading

@sandreenko sandreenko self-requested a review Sep 23, 2019
Copy link

@sandreenko sandreenko left a comment

the change looks good, thank you!

Sorry , it took me longer than I expected to reach that PR.
Could you please rebase/fix the conflicts and we will merge that?

Loading

@EgorBo
Copy link
Author

@EgorBo EgorBo commented Oct 16, 2019

@sandreenko sure!

Loading

@EgorBo EgorBo force-pushed the jit-unsinged-comp-fixes branch from ac7842b to 55619df Oct 16, 2019
@EgorBo EgorBo force-pushed the jit-unsinged-comp-fixes branch from 55619df to 5ea3065 Oct 16, 2019
@sandreenko
Copy link

@sandreenko sandreenko commented Oct 16, 2019

@EgorBo could you please clone https://github.com/dotnet/jitutils, run bootstrap.cmd and then run jit-format to fix the formatting errors, like:

seandree@SEANDREEPC C:\git\tools\jitutils\bin
$ jit-format.bat --fix -c E:\git\coreclr

Loading

@sandreenko sandreenko merged commit 5800248 into dotnet:master Oct 16, 2019
53 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants