JIT: Improve constant folding for bitwise OR #27325

EgorBo · 2019-10-21T00:26:25Z

This PR re-uses GT_ADD constant folding logic for GT_OR in morph.cpp.
Example:

int Test1(int x) => x | 5 | 3; // e.g. enum flags

int Test2(int x, int y) => (x | 5) | (y | 3);

Was:

; Test1(int,int):int:this
       mov      eax, edx
       or       eax, 5
       or       eax, 3
       ret      
; Total bytes of code: 9


; Test2(int,int):int:this
       or       edx, 5
       mov      eax, edx
       or       eax, r8d
       or       eax, 3
       ret      
; Total bytes of code: 12

Now:

; Test1(int,int):int:this
       mov      eax, edx
       or       eax, 7
       ret      
; Total bytes of code: 6


; Test2(int,int):int:this
       mov      eax, edx
       or       eax, r8d
       or       eax, 7
       ret      
; Total bytes of code: 9

Jit-diff:

Found 8 files with textual diffs.

Summary:
(Lower is better)

Total bytes of diff: -148 (-0.00% of base)
    diff is an improvement.

Top file improvements by size (bytes):
         -59 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.00% of base)
         -54 : Microsoft.CodeAnalysis.CSharp.dasm (-0.00% of base)
         -16 : System.Memory.dasm (-0.00% of base)
          -7 : System.Private.Uri.dasm (-0.01% of base)
          -5 : System.Private.Xml.dasm (-0.00% of base)
          -4 : System.Net.Security.dasm (-0.00% of base)
          -3 : System.Data.Common.dasm (-0.00% of base)

7 total files with size differences (7 improved, 0 regressed), 122 unchanged.

Top method improvements by size (bytes):
         -28 (-6.81% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.Symbols.SourceEventSymbol:BindEventAccessor(ref,ref):ref:this
         -14 (-0.89% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.MemberSemanticModel:GetEnclosingBinder(ref,int):ref:this
          -8 (-0.82% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - ConversionEasyOut:ClassifyPredefinedConversion(ref,ref):struct
          -7 (-1.42% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.Binder:BindTypeOf(ref,ref):ref:this
          -7 (-0.30% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.Symbols.SourceMethodSymbol:CreateDeclareMethod(ref,ref,ref,ref):ref
          -7 (-9.86% of base) : System.Private.Uri.dasm - BuiltInUriParser:.ctor(ref,int,int):this
          -6 (-0.16% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.OverloadResolution:GetEnumOperation(int,ref,ref,ref,ref):this
          -6 (-7.89% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.TypeofBinder:.ctor(ref,ref):this
          -6 (-2.03% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.CSharpSemanticModel:GetSpeculativeBinder(int,ref,int):ref:this
          -6 (-0.23% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.Symbols.SourcePropertySymbol:Create(ref,ref,ref,ref,ref):ref
          -6 (-1.23% of base) : System.Memory.dasm - System.Buffers.Text.Base64:EncodeToUtf8InPlace(struct,int,byref):int
          -5 (-0.40% of base) : System.Memory.dasm - System.Buffers.Text.Base64:EncodeToUtf8(struct,struct,byref,byref,bool):int
          -5 (-12.20% of base) : System.Memory.dasm - System.Buffers.Text.Base64:EncodeAndPadTwo(long,byref):int
          -5 (-0.32% of base) : System.Private.Xml.dasm - System.Xml.Serialization.XmlSerializationILGen:GenerateSerializerContract(ref,ref,ref,ref,ref,ref,ref,ref):this
          -4 (-0.43% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - MemberLookup:LookupInModules(ref,ref,ref,int,int,ref,byref)

Top method improvements by size (percentage):
          -5 (-12.20% of base) : System.Memory.dasm - System.Buffers.Text.Base64:EncodeAndPadTwo(long,byref):int
          -7 (-9.86% of base) : System.Private.Uri.dasm - BuiltInUriParser:.ctor(ref,int,int):this
          -6 (-7.89% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.TypeofBinder:.ctor(ref,ref):this
          -3 (-7.69% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.WithTypeParametersBinder:CanConsiderTypeParameters(int):bool:this
          -3 (-7.69% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.Symbols.SourceMemberMethodSymbol:AddImpliedModifiers(int,bool,int):int
         -28 (-6.81% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.Symbols.SourceEventSymbol:BindEventAccessor(ref,ref):ref:this
          -4 (-2.40% of base) : System.Net.Security.dasm - System.Net.Security.SecureChannel:GetAlertMessageFromChain(ref):int
          -6 (-2.03% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.CSharpSemanticModel:GetSpeculativeBinder(int,ref,int):ref:this
          -3 (-2.01% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.Symbols.SourceMethodSymbol:GetPInvokeAttributes(ref):ushort
          -3 (-1.48% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.Symbols.SourceEventSymbol:MakeModifiers(struct,bool,ref,ref,byref):int:this
          -3 (-1.42% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.Symbols.SourcePropertySymbol:MakeModifiers(struct,bool,bool,ref,ref,byref):int:this
          -7 (-1.42% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.Binder:BindTypeOf(ref,ref):ref:this
          -3 (-1.33% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.Symbols.Metadata.PE.PEMethodSymbol:IsParameterlessConstructor():bool:this
          -6 (-1.23% of base) : System.Memory.dasm - System.Buffers.Text.Base64:EncodeToUtf8InPlace(struct,int,byref):int
         -14 (-0.89% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.MemberSemanticModel:GetEnclosingBinder(ref,int):ref:this

24 total methods with size differences (24 improved, 0 regressed), 250171 unchanged.

maryamariyan · 2019-11-06T21:03:29Z

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

In your coreclr repository clone, create patch by running git format-patch origin
In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>

mikedn · 2019-11-07T21:35:33Z

src/jit/morph.cpp

            /* See if we can fold GT_ADD nodes. */

-            if (oper == GT_ADD)
+            if (oper == GT_ADD || oper == GT_OR)


Why not add GT_MUL, GT_AND and GT_XOR as well?

Also, the comment above should be changed - probably to:

// See if we can fold constants for commutative operators.

@mikedn good point, I was only keeping in mind the Enum and OR case (it's quite popular) didn't want to touch GT_MUL due to possible overflows, but will re-consider thanks to feedback

briansull · 2019-11-07T21:45:31Z

src/jit/morph.cpp

+                        }
+                        else
+                        {
+                            cns1->AsIntCon()->SetIconValue(icon1 | icon2);


Should have an assert that the oper is correct: GT_OR

That follows from the surrounding if-clause

briansull · 2019-11-07T21:46:09Z

src/jit/morph.cpp

+                        }
+                        else
+                        {
+                            op2->AsIntConCommon()->SetIconValue(icon1 | icon2);


Should have an assert that the oper is correct: GT_OR

CarolEidt · 2019-11-07T22:21:23Z

src/jit/morph.cpp

            /* See if we can fold GT_ADD nodes. */

-            if (oper == GT_ADD)
+            if (oper == GT_ADD || oper == GT_OR)


Also, the comment above should be changed - probably to:

// See if we can fold constants for commutative operators.

CarolEidt · 2019-11-07T22:23:59Z

src/jit/morph.cpp

+            if (oper == GT_ADD || oper == GT_OR)
            {
-                /* Fold "((x+icon1)+(y+icon2)) to ((x+y)+(icon1+icon2))" */
+                /* Fold "((x+icon1)+(y+icon2)) to ((x+y)+(icon1+icon2))" same for bitwise OR */


The same for bitwise OR is not really very descriptive. Probably would be better to say something like:

// Fold "(x <op> icon1) <op> (y <op> icon2))" to "((x <op> y) <op> (icon1 <op> icon2))"

EgorBo · 2019-11-22T17:04:20Z

Thank you for feedback, will address it and manually port the PR to the new repo once it lands.

maryamariyan · 2019-12-02T19:37:19Z

Thank you for your contribution. As announced in #27549 the dotnet/runtime repository will be used going forward for changes to this code base. Closing this PR as no more changes will be accepted into master for this repository. If you’d like to continue working on this change please move it to dotnet/runtime.

EgorBo added 4 commits October 21, 2019 02:46

Improve constant folding for bitwise OR

d6d8ab7

forgot fgRecognizeAndMorphBitwiseRotation

de69782

code formatting

997fef4

code formatting

6541d97

sandreenko added the area-CodeGen label Oct 21, 2019

code formatting

cffd873

adamsitnik added the tenet-performance Performance related issue label Oct 21, 2019

BruceForstall added the post-consolidation PRs which will be hand ported to dotnet/runtime label Nov 7, 2019

BruceForstall requested a review from a team November 7, 2019 21:32

mikedn reviewed Nov 7, 2019

View reviewed changes

briansull reviewed Nov 7, 2019

View reviewed changes

src/jit/morph.cpp

}

else

{

op2->AsIntConCommon()->SetIconValue(icon1 | icon2);

Copy link

briansull Nov 7, 2019

Choose a reason for hiding this comment

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

Should have an assert that the oper is correct: GT_OR

CarolEidt suggested changes Nov 7, 2019

View reviewed changes

maryamariyan closed this Dec 2, 2019

EgorBo mentioned this pull request Dec 7, 2019

JIT: Improve constant folding for commutative operators dotnet/runtime#656

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

JIT: Improve constant folding for bitwise OR #27325

JIT: Improve constant folding for bitwise OR #27325

Uh oh!

EgorBo commented Oct 21, 2019 •

edited

Loading

Uh oh!

maryamariyan commented Nov 6, 2019

Uh oh!

mikedn Nov 7, 2019

Uh oh!

CarolEidt Nov 7, 2019

Uh oh!

EgorBo Nov 8, 2019 •

edited

Loading

Uh oh!

briansull Nov 7, 2019

Uh oh!

CarolEidt Nov 7, 2019

Uh oh!

briansull Nov 7, 2019

Uh oh!

CarolEidt Nov 7, 2019

Uh oh!

CarolEidt Nov 7, 2019

Uh oh!

EgorBo commented Nov 22, 2019

Uh oh!

maryamariyan commented Dec 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

JIT: Improve constant folding for bitwise OR #27325

JIT: Improve constant folding for bitwise OR #27325

Uh oh!

Conversation

EgorBo commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maryamariyan commented Nov 6, 2019

Uh oh!

mikedn Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

CarolEidt Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

EgorBo Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansull Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

CarolEidt Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

briansull Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

CarolEidt Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

CarolEidt Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Nov 22, 2019

Uh oh!

maryamariyan commented Dec 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

EgorBo commented Oct 21, 2019 •

edited

Loading

EgorBo Nov 8, 2019 •

edited

Loading