Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Jit: fix for broken type equality optimization #9562

Merged
merged 1 commit into from
Feb 13, 2017

Conversation

AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Feb 13, 2017

The jit relies on a particular tree remaining intact during importation
to optimize type equality tests commonly found in generics.

In #7907 the jit started aggressively spilling the evaluation stack before
calls, which broke up the trees needed for the optimizations.

In the special cases pertaining to type tests the trees can safely remain
intact because we know the calls can't interact with the computations on
the stack. Recognize these cases and selectively suppress spilling.

Closes #9552.

The jit relies on a particular tree remaining intact during importation
to optimize type equality tests commonly found in generics.

In dotnet#7909 the jit started aggressively spilling the evaluation stack before
calls, which broke up the trees needed for the optimizations.

In the special cases pertaining to type tests the trees can safely remain
intact because we know the calls can't interact with the computations on
the stack. Recognize these cases and selectively suppress spilling.

Closes #9552.
@AndyAyersMS
Copy link
Member Author

@JosephTremoulet PTAL
cc @dotnet/jit-contrib

Fixes the case in #9552.

Code size diffs:

Total bytes of diff: -19089 (-0.14 % of base)
    diff is an improvement.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file regressions by size (bytes):
          18 : Microsoft.VisualBasic.dasm (0.01 % of base)
           3 : System.Diagnostics.DiagnosticSource.dasm (0.04 % of base)
Top file improvements by size (bytes):
      -16068 : System.Private.CoreLib.dasm (-0.45 % of base)
       -1917 : System.Linq.Parallel.dasm (-0.31 % of base)
        -322 : Microsoft.CodeAnalysis.dasm (-0.04 % of base)
        -278 : System.Runtime.Extensions.dasm (-0.17 % of base)
        -206 : System.Net.Sockets.dasm (-0.12 % of base)
15 total files with size differences (13 improved, 2 regressed).
Top method regessions by size (bytes):
          10 : Microsoft.VisualBasic.dasm - Microsoft.VisualBasic.CompilerServices.VBCallBinder:FallbackInvokeMember(ref,ref,ref):ref:this
          10 : System.Linq.Expressions.dasm - MetaDynamic:BuildCallMethodWithResult(ref,ref,ref,ref,ref):ref:this
           8 : Microsoft.VisualBasic.dasm - Microsoft.VisualBasic.CompilerServices.VBIndexSetComplexBinder:FallbackSetIndex(ref,ref,ref,ref):ref:this
           8 : System.Private.CoreLib.dasm - System.Exception:GetExceptionMethodFromString():ref:this
           7 : System.Linq.Expressions.dasm - System.Linq.Expressions.Expression:ValidateIndexedProperty(ref,ref,ref,byref)
Top method improvements by size (bytes):
       -2580 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.CLRIPropertyValueImpl:CoerceScalarValue(int,ref):long (2 methods)
       -2498 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.CLRIPropertyValueImpl:CoerceScalarValue(int,ref):int (2 methods)
       -1917 : System.Linq.Parallel.dasm - System.Linq.Parallel.Util:GetDefaultComparer():ref (13 methods)
       -1325 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.CLRIPropertyValueImpl:CoerceScalarValue(int,ref):double
       -1325 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.CLRIPropertyValueImpl:CoerceScalarValue(int,ref):float
107 total methods with size differences (98 improved, 9 regressed).

Copy link

@JosephTremoulet JosephTremoulet left a comment

Choose a reason for hiding this comment

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

LGTM, but worth considering whether to generalize the helper call case to check some subset of these flags

@stephentoub
Copy link
Member

Thanks, @JosephTremoulet and @AndyAyersMS! I applied Joseph's change and looked at the case in AsyncTaskMethodBuilder I cared about.... significant drop in the amount of assembly, though there was still one CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE call remaining that looked unnecessary. Then I applied Andy's change as well, and it went away. Everything looks good to me now. Thanks!

@AndyAyersMS
Copy link
Member Author

This isn't the only place in the importer that can spill so perhaps we can use special knowledge of the callee more generally to pare down the side effect flags for GT_CALL nodes and reduce interference in more cases. Will look at that separately.

@AndyAyersMS AndyAyersMS merged commit 965d079 into dotnet:master Feb 13, 2017
@AndyAyersMS AndyAyersMS deleted the FixMagicTypeEquality branch February 13, 2017 22:52
@JosephTremoulet
Copy link

there was still one CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE call remaining that looked unnecessary. Then I applied Andy's change as well, and it went away.

Probably in general dead-code elimination we're conservative about removing dead calls for fear of side-effects, and the optimization in morph is more aggressive. I wonder how many unnecessary helper calls get left around...

@briansull
Copy link

Note that we don't value number blocks that are unreachable at all.

Thus if this pattern occurs in an unreachable block we won't eliminate it via identical value numbers.

This leads to a A/V in our new JIT in some customer code that contains a loop that is entirely unreachable,when we try to unpack value numbers to see if we can eliminate a null check.

@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…quality

Jit: fix for broken type equality optimization

Commit migrated from dotnet/coreclr@965d079
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants