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

Use local assertion prop to omit casts on returns #55632

Conversation

SingleAccretion
Copy link
Contributor

When inserting normalizing casts for returns in pre-order
morph, fgMorphCast was used, which did not account for
the fact that local assertion propagation could tell us
that the cast is in fact unnecessary. Fix this by calling
fgMorphTree instead.

Also some miscellaneous code cleanup.

The diffs are almost non-existent, but are there:

coreclr_tests.pmi.windows.x64.checked.mch:


Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 1700
Total bytes of diff: 1655
Total bytes of delta: -45 (-2.65% of base)
Total relative delta: -0.68
    diff is an improvement.
    relative diff is an improvement.
Detail diffs


Top file improvements (bytes):
          -3 : 80797.dasm (-2.59% of base)
          -2 : 237399.dasm (-2.70% of base)
          -2 : 237455.dasm (-2.70% of base)
          -2 : 237376.dasm (-3.85% of base)
          -2 : 237442.dasm (-3.85% of base)
          -2 : 237389.dasm (-2.70% of base)
          -2 : 237443.dasm (-3.85% of base)
          -2 : 237456.dasm (-2.56% of base)
          -2 : 237465.dasm (-2.70% of base)
          -2 : 237393.dasm (-2.82% of base)
          -2 : 237390.dasm (-2.56% of base)
          -2 : 237403.dasm (-2.82% of base)
          -2 : 237459.dasm (-2.82% of base)
          -2 : 237466.dasm (-2.56% of base)
          -2 : 237377.dasm (-3.85% of base)
          -2 : 237400.dasm (-2.56% of base)
          -2 : 237448.dasm (-3.85% of base)
          -2 : 237382.dasm (-3.85% of base)
          -2 : 237469.dasm (-2.82% of base)
          -1 : 237394.dasm (-1.43% of base)

25 total files with Code Size differences (25 improved, 0 regressed), 0 unchanged.

Top method improvements (bytes):
          -3 (-2.59% of base) : 80797.dasm - Repro:ReadUInt16():ushort
          -2 (-2.70% of base) : 237399.dasm - ConvTest:test24():byte
          -2 (-2.70% of base) : 237455.dasm - ConvTest:test14():byte
          -2 (-3.85% of base) : 237376.dasm - ConvTest:test1():byte
          -2 (-3.85% of base) : 237442.dasm - ConvTest:test1():byte
          -2 (-2.70% of base) : 237389.dasm - ConvTest:test14():byte
          -2 (-3.85% of base) : 237443.dasm - ConvTest:test2():short
          -2 (-2.56% of base) : 237456.dasm - ConvTest:test15():short
          -2 (-2.70% of base) : 237465.dasm - ConvTest:test24():byte
          -2 (-2.82% of base) : 237393.dasm - ConvTest:test18():ubyte
          -2 (-2.56% of base) : 237390.dasm - ConvTest:test15():short
          -2 (-2.82% of base) : 237403.dasm - ConvTest:test28():ubyte
          -2 (-2.82% of base) : 237459.dasm - ConvTest:test18():ubyte
          -2 (-2.56% of base) : 237466.dasm - ConvTest:test25():short
          -2 (-3.85% of base) : 237377.dasm - ConvTest:test2():short
          -2 (-2.56% of base) : 237400.dasm - ConvTest:test25():short
          -2 (-3.85% of base) : 237448.dasm - ConvTest:test7():ubyte
          -2 (-3.85% of base) : 237382.dasm - ConvTest:test7():ubyte
          -2 (-2.82% of base) : 237469.dasm - ConvTest:test28():ubyte
          -1 (-1.43% of base) : 237394.dasm - ConvTest:test19():ushort

Top method improvements (percentages):
          -2 (-3.85% of base) : 237376.dasm - ConvTest:test1():byte
          -2 (-3.85% of base) : 237442.dasm - ConvTest:test1():byte
          -2 (-3.85% of base) : 237443.dasm - ConvTest:test2():short
          -2 (-3.85% of base) : 237377.dasm - ConvTest:test2():short
          -2 (-3.85% of base) : 237448.dasm - ConvTest:test7():ubyte
          -2 (-3.85% of base) : 237382.dasm - ConvTest:test7():ubyte
          -2 (-2.82% of base) : 237393.dasm - ConvTest:test18():ubyte
          -2 (-2.82% of base) : 237403.dasm - ConvTest:test28():ubyte
          -2 (-2.82% of base) : 237459.dasm - ConvTest:test18():ubyte
          -2 (-2.82% of base) : 237469.dasm - ConvTest:test28():ubyte
          -2 (-2.70% of base) : 237399.dasm - ConvTest:test24():byte
          -2 (-2.70% of base) : 237455.dasm - ConvTest:test14():byte
          -2 (-2.70% of base) : 237389.dasm - ConvTest:test14():byte
          -2 (-2.70% of base) : 237465.dasm - ConvTest:test24():byte
          -3 (-2.59% of base) : 80797.dasm - Repro:ReadUInt16():ushort
          -2 (-2.56% of base) : 237456.dasm - ConvTest:test15():short
          -2 (-2.56% of base) : 237390.dasm - ConvTest:test15():short
          -2 (-2.56% of base) : 237466.dasm - ConvTest:test25():short
          -2 (-2.56% of base) : 237400.dasm - ConvTest:test25():short
          -1 (-2.00% of base) : 237383.dasm - ConvTest:test8():ushort

25 total methods with Code Size differences (25 improved, 0 regressed), 0 unchanged.


libraries.crossgen2.windows.x64.checked.mch:


Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 521
Total bytes of diff: 506
Total bytes of delta: -15 (-2.88% of base)
Total relative delta: -0.17
    diff is an improvement.
    relative diff is an improvement.
Detail diffs


Top file improvements (bytes):
          -4 : 131557.dasm (-5.48% of base)
          -3 : 131554.dasm (-4.23% of base)
          -3 : 131553.dasm (-4.23% of base)
          -3 : 90249.dasm (-1.50% of base)
          -2 : 33831.dasm (-1.89% of base)

5 total files with Code Size differences (5 improved, 0 regressed), 0 unchanged.

Top method improvements (bytes):
          -4 (-5.48% of base) : 131557.dasm - Microsoft.VisualBasic.CompilerServices.VB6File:GetShort(long):short:this
          -3 (-4.23% of base) : 131554.dasm - Microsoft.VisualBasic.CompilerServices.VB6File:GetByte(long):ubyte:this
          -3 (-4.23% of base) : 131553.dasm - Microsoft.VisualBasic.CompilerServices.VB6File:GetChar(long):ushort:this
          -3 (-1.50% of base) : 90249.dasm - Newtonsoft.Json.JsonTextReader:ConvertUnicode(bool):ushort:this
          -2 (-1.89% of base) : 33831.dasm - Microsoft.CodeAnalysis.ArrayBuilderExtensions:Pop(Microsoft.CodeAnalysis.ArrayBuilder`1[Microsoft.CodeAnalysis.SyntaxNode+TwoEnumeratorListStack+Which]):ubyte

Top method improvements (percentages):
          -4 (-5.48% of base) : 131557.dasm - Microsoft.VisualBasic.CompilerServices.VB6File:GetShort(long):short:this
          -3 (-4.23% of base) : 131554.dasm - Microsoft.VisualBasic.CompilerServices.VB6File:GetByte(long):ubyte:this
          -3 (-4.23% of base) : 131553.dasm - Microsoft.VisualBasic.CompilerServices.VB6File:GetChar(long):ushort:this
          -2 (-1.89% of base) : 33831.dasm - Microsoft.CodeAnalysis.ArrayBuilderExtensions:Pop(Microsoft.CodeAnalysis.ArrayBuilder`1[Microsoft.CodeAnalysis.SyntaxNode+TwoEnumeratorListStack+Which]):ubyte
          -3 (-1.50% of base) : 90249.dasm - Newtonsoft.Json.JsonTextReader:ConvertUnicode(bool):ushort:this

5 total methods with Code Size differences (5 improved, 0 regressed), 0 unchanged.


libraries.pmi.windows.x64.checked.mch:


Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 348
Total bytes of diff: 341
Total bytes of delta: -7 (-2.01% of base)
Total relative delta: -0.08
    diff is an improvement.
    relative diff is an improvement.
Detail diffs


Top file improvements (bytes):
          -3 : 105662.dasm (-1.32% of base)
          -2 : 76226.dasm (-3.33% of base)
          -2 : 76225.dasm (-3.33% of base)

3 total files with Code Size differences (3 improved, 0 regressed), 0 unchanged.

Top method improvements (bytes):
          -3 (-1.32% of base) : 105662.dasm - Newtonsoft.Json.JsonTextReader:ConvertUnicode(bool):ushort:this
          -2 (-3.33% of base) : 76226.dasm - Microsoft.CodeAnalysis.ArrayBuilderExtensions:Pop(Microsoft.CodeAnalysis.ArrayBuilder`1[Int16]):short
          -2 (-3.33% of base) : 76225.dasm - Microsoft.CodeAnalysis.ArrayBuilderExtensions:Pop(Microsoft.CodeAnalysis.ArrayBuilder`1[Byte]):ubyte

Top method improvements (percentages):
          -2 (-3.33% of base) : 76226.dasm - Microsoft.CodeAnalysis.ArrayBuilderExtensions:Pop(Microsoft.CodeAnalysis.ArrayBuilder`1[Int16]):short
          -2 (-3.33% of base) : 76225.dasm - Microsoft.CodeAnalysis.ArrayBuilderExtensions:Pop(Microsoft.CodeAnalysis.ArrayBuilder`1[Byte]):ubyte
          -3 (-1.32% of base) : 105662.dasm - Newtonsoft.Json.JsonTextReader:ConvertUnicode(bool):ushort:this

3 total methods with Code Size differences (3 improved, 0 regressed), 0 unchanged.


libraries_tests.pmi.windows.x64.checked.mch:


Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 732
Total bytes of diff: 718
Total bytes of delta: -14 (-1.91% of base)
Total relative delta: -0.15
    diff is an improvement.
    relative diff is an improvement.
Detail diffs


Top file improvements (bytes):
          -4 : 128650.dasm (-1.95% of base)
          -3 : 128649.dasm (-1.48% of base)
          -2 : 11111.dasm (-3.33% of base)
          -2 : 11110.dasm (-3.33% of base)
          -1 : 118419.dasm (-2.27% of base)
          -1 : 269986.dasm (-1.27% of base)
          -1 : 269988.dasm (-1.23% of base)

7 total files with Code Size differences (7 improved, 0 regressed), 0 unchanged.

Top method improvements (bytes):
          -4 (-1.95% of base) : 128650.dasm - Microsoft.Diagnostics.Runtime.Linux.Reader:Read(byref):short:this
          -3 (-1.48% of base) : 128649.dasm - Microsoft.Diagnostics.Runtime.Linux.Reader:Read(byref):ubyte:this
          -2 (-3.33% of base) : 11111.dasm - Microsoft.CodeAnalysis.ArrayBuilderExtensions:Pop(Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder`1[Int16]):short
          -2 (-3.33% of base) : 11110.dasm - Microsoft.CodeAnalysis.ArrayBuilderExtensions:Pop(Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder`1[Byte]):ubyte
          -1 (-2.27% of base) : 118419.dasm - Microsoft.Build.Shared.LanguageParser.TokenCharReader:SinkCharacter():ushort:this
          -1 (-1.27% of base) : 269986.dasm - Stashbox.Utils.Data.Stack`1[Byte][System.Byte]:Pop():ubyte:this
          -1 (-1.23% of base) : 269988.dasm - Stashbox.Utils.Data.Stack`1[Int16][System.Int16]:Pop():short:this

Top method improvements (percentages):
          -2 (-3.33% of base) : 11111.dasm - Microsoft.CodeAnalysis.ArrayBuilderExtensions:Pop(Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder`1[Int16]):short
          -2 (-3.33% of base) : 11110.dasm - Microsoft.CodeAnalysis.ArrayBuilderExtensions:Pop(Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder`1[Byte]):ubyte
          -1 (-2.27% of base) : 118419.dasm - Microsoft.Build.Shared.LanguageParser.TokenCharReader:SinkCharacter():ushort:this
          -4 (-1.95% of base) : 128650.dasm - Microsoft.Diagnostics.Runtime.Linux.Reader:Read(byref):short:this
          -3 (-1.48% of base) : 128649.dasm - Microsoft.Diagnostics.Runtime.Linux.Reader:Read(byref):ubyte:this
          -1 (-1.27% of base) : 269986.dasm - Stashbox.Utils.Data.Stack`1[Byte][System.Byte]:Pop():ubyte:this
          -1 (-1.23% of base) : 269988.dasm - Stashbox.Utils.Data.Stack`1[Int16][System.Int16]:Pop():short:this

7 total methods with Code Size differences (7 improved, 0 regressed), 0 unchanged.


Example of the expected improvement, from Microsoft.CodeAnalysis.ArrayBuilderExtensions:Pop:

G_M1359_IG01:
       push     rdi
       push     rsi
       sub      rsp, 40
       mov      rsi, rcx
G_M1359_IG02:
       mov      rcx, gword ptr [rsi+8]
       mov      rdx, rcx
       mov      edx, dword ptr [rdx+16]
       dec      edx
       call     [Builder[Int16][System.Int16]:get_Item(int):short:this]
       movsx    rdi, ax
       mov      rcx, gword ptr [rsi+8]
       mov      rdx, rcx
       mov      edx, dword ptr [rdx+16]
       dec      edx
       call     [Builder[Int16][System.Int16]:RemoveAt(int):this]
-      movsx    rax, di
+      mov      eax, edi
G_M1359_IG03:
       add      rsp, 40
       pop      rsi
       pop      rdi
       ret

When inserting normalizing casts for returns in pre-order
morph, "fgMorphCast" was used, which did not account for
the fact that local assertion propagation could tell us
that the cast is in fact unnecessary. Fix this by calling
"fgMorphTree" instead.

Also some miscellaneous code cleanup.
@dotnet-issue-labeler 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 14, 2021
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

@kunalspathak kunalspathak merged commit 9cfb02a into dotnet:main Jul 15, 2021
@SingleAccretion SingleAccretion deleted the Elide-Normalizing-Casts-Via-Local-Assertion-Prop branch July 16, 2021 08:18
@ghost ghost locked as resolved and limited conversation to collaborators Aug 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants