Value number TypeHandleToRuntimeType helper by JosephTremoulet · Pull Request #9560 · dotnet/coreclr

JosephTremoulet · 2017-02-13T19:11:48Z

This is a pure helper w/o side-effects, so add it to the lists of
tractable helpers in value-numbering; this allows redundant calls to be
CSEd, and fixes #9552 so we can again optimize away type checks on type
parameters in generic code (a not-infrequent pattern).

This is a pure helper w/o side-effects, so add it to the lists of tractable helpers in value-numbering; this allows redundant calls to be CSEd, and fixes #9552 so we can again optimize away type checks on type parameters in generic code (a not-infrequent pattern).

JosephTremoulet · 2017-02-13T19:12:23Z

@AndyAyersMS @briansull @dotnet/jit-contrib PTAL.

pgavlin

LGTM

AndyAyersMS · 2017-02-13T19:40:21Z

Do we know for sure it is pure? Or is it only pure for the special cases that the pattern match was getting?

Would like to see if you get any diffs from this in jit-diff.

JosephTremoulet · 2017-02-13T19:51:03Z

Do we know for sure it is pure? Or is it only pure for the special cases that the pattern match was getting?

It looks to me like the pattern-match is happy with any CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE call it can find, the heuristics are about how it's consumed rather than how it's produced. I suppose it's possible there's some hidden contract in the pattern-match?

Maybe @jkotas or @davidwrighton or someone can answer more authoritatively: is it safe for the JIT to always assume that CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE is pure and non-throwing?

jkotas · 2017-02-13T20:07:14Z

is it safe for the JIT to always assume that CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE is pure and non-throwing?

Yes. It can throw out-of-memory - but it should not stop you from treating it as pure and side-effect free.

mikedn · 2017-02-13T20:10:05Z

Should the relevant morph code be deleted?

JosephTremoulet · 2017-02-13T20:11:25Z

@dotnet-bot test OSX x64 Checked Build and Test

pgavlin · 2017-02-13T20:14:06Z

Should the relevant morph code be deleted?

I would recommend leaving the code in morph if @AndyAyersMS's pending work makes it effective again, as it may be useful during scenarios that do not run with the SSA-based optimizer enabled.

JosephTremoulet · 2017-02-13T20:14:25Z

Should the relevant morph code be deleted?

@AndyAyersMS indicated that he's planning to update the importer in a way that the morph code would again be catching most (all?) of these changes. My thinking is the redundancy isn't much of a maintenance or throughput concern, so may as well have the morph code to snip out most cases early and the value numbering to pick up any stragglers or new/different patterns.

AndyAyersMS · 2017-02-13T20:18:12Z

Yeah, I think we should do both; the space & size savings are significant and the morph pattern kicks in unconditionally, and it's pretty cheap (in terms of jit time) to recognize. The less code the optimizer has to look at the better.

Will be interesting to see if the VN approach finds any additional instances.

JosephTremoulet · 2017-02-13T23:19:08Z

Would like to see if you get any diffs from this in jit-diff

I do, but if any of them are from optimizing away this sort of compare, they get drowned out because most/all diffs are straight CSEs of redundant calls to this helper, not on two sides of a compare. Here's the jit-analyze output:

Summary:
(Note: Lower is better)

Total bytes of diff: -9248 (-0.07 % 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 improvements by size (bytes):
       -8110 : System.Private.CoreLib.dasm (-0.23 % of base)
        -582 : System.Linq.Parallel.dasm (-0.09 % of base)
        -188 : System.Reflection.Metadata.dasm (-0.05 % of base)
        -178 : System.Linq.Expressions.dasm (-0.03 % of base)
        -113 : Microsoft.CSharp.dasm (-0.03 % of base)

9 total files with size differences (9 improved, 0 regressed).

Top method regessions by size (bytes):
          37 : System.Private.CoreLib.dasm - System.Diagnostics.Tracing.ManifestBuilder:CreateManifestString():ref:this
          18 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.CLRIPropertyValueImpl:CoerceScalarValue(int,ref):ref
           8 : System.Private.CoreLib.dasm - System.Diagnostics.Tracing.EventSource:WriteMultiMerge(ref,byref,ref,long,long,long):this
           7 : Microsoft.CodeAnalysis.dasm - Roslyn.Utilities.ObjectWriter:WriteValue(ref):this
           7 : Microsoft.CSharp.dasm - Microsoft.CSharp.RuntimeBinder.Semantics.CType:IsCollectionType():bool:this

Top method improvements by size (bytes):
       -1286 : System.Private.CoreLib.dasm - System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1[Boolean][System.Boolean]:SetResult(bool):this
       -1133 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.CLRIPropertyValueImpl:CoerceScalarValue(int,ref):int (2 methods)
        -935 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.CLRIPropertyValueImpl:CoerceScalarValue(int,ref):ubyte
        -833 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.CLRIPropertyValueImpl:CoerceScalarValue(int,ref):short
        -780 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.CLRIPropertyValueImpl:CoerceScalarValue(int,ref):long (2 methods)

117 total methods with size differences (92 improved, 25 regressed).

Will be interesting to see if the VN approach finds any additional instances

Presumably these CSE cases get past the compare-centric morph optimization.

AndyAyersMS · 2017-02-13T23:48:46Z

Above data is from running on top of #9562... ?

stephentoub · 2017-02-13T23:53:16Z

Do we have any tests to help catch regressions in this kind of thing in the future?

AndyAyersMS · 2017-02-14T00:00:52Z

For code size and such there is manual testing available via jit-diff; in fact I ran it for #7923 and commented as such in the PR. We usually ask jit devs to post this data.

Evidently I did something wrong when I ran the tests.

JosephTremoulet · 2017-02-14T00:07:51Z

Above data is from running on top of #9562... ?

No, running before that got merged.

AndyAyersMS · 2017-02-14T00:40:03Z

The data I get on top of #9562:

Summary:
(Note: Lower is better)

Total bytes of diff: -2631 (-0.02 % 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 improvements by size (bytes):
       -2045 : System.Private.CoreLib.dasm (-0.06 % of base)
        -188 : System.Reflection.Metadata.dasm (-0.05 % of base)
        -173 : System.Linq.Expressions.dasm (-0.03 % of base)
        -134 : Microsoft.CSharp.dasm (-0.03 % of base)
         -69 : System.Linq.Queryable.dasm (-0.11 % of base)

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

Top method regessions by size (bytes):
          37 : System.Private.CoreLib.dasm - System.Diagnostics.Tracing.ManifestBuilder:CreateManifestString():ref:this
          35 : System.Private.CoreLib.dasm - System.Runtime.InteropServices.WindowsRuntime.CLRIPropertyValueImpl:CoerceScalarValue(int,ref):ref
          12 : System.Linq.Expressions.dasm - System.Linq.Expressions.Expression:ValidateGoto(ref,byref,ref,ref,ref)
           7 : Microsoft.CodeAnalysis.dasm - Roslyn.Utilities.ObjectWriter:WriteValue(ref):this
           5 : System.Private.CoreLib.dasm - System.Diagnostics.Tracing.EventSource:WriteMultiMerge(ref,byref,ref,long,long,long):this

Top method improvements by size (bytes):
        -442 : System.Private.CoreLib.dasm - System.Collections.Generic.EqualityComparer`1[Byte][System.Byte]:CreateComparer():ref
        -330 : System.Private.CoreLib.dasm - System.Runtime.CompilerServices.JitHelpers:UnsafeEnumCast(int):int (4 methods)
         -96 : Microsoft.CSharp.dasm - Microsoft.CSharp.RuntimeBinder.SymbolTable:SetParameterAttributes(ref,ref,int):this
         -95 : System.Private.CoreLib.dasm - System.Exception:GetObjectData(ref,struct):this
         -94 : System.Reflection.Metadata.dasm - System.Reflection.Metadata.BlobWriterImpl:WriteConstant(byref,ref)

124 total methods with size differences (99 improved, 25 regressed).

AndyAyersMS · 2017-02-14T00:52:28Z

Diffs in CreateComparer are a great example why we want the in-depth aspect of both changes:

            object result = null;
            RuntimeType t = (RuntimeType)typeof(T);
            
            // Specialize type byte for performance reasons
            if (t == typeof(byte)) {
                result = new ByteEqualityComparer();
            }

The code author likely thought hosting typeof(T) into a local like this was an optimization, but it breaks the tree-based pattern optimization.

BTW the generated code -- while vastly simpler -- still looks overly complex to me...

Value number TypeHandleToRuntimeType helper Commit migrated from dotnet/coreclr@539b142

dnfclas added the cla-already-signed label Feb 13, 2017

pgavlin approved these changes Feb 13, 2017

View reviewed changes

stephentoub mentioned this pull request Feb 13, 2017

Reduce Task and async method overheads #9471

Merged

JosephTremoulet merged commit 539b142 into dotnet:master Feb 13, 2017

JosephTremoulet deleted the TypeToTypeVN branch February 13, 2017 23:38

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

Merge pull request dotnet/coreclr#9560 from JosephTremoulet/TypeToTypeVN

17f0ed4

Value number TypeHandleToRuntimeType helper Commit migrated from dotnet/coreclr@539b142

Conversation

JosephTremoulet commented Feb 13, 2017

Uh oh!

JosephTremoulet commented Feb 13, 2017

Uh oh!

pgavlin left a comment

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Feb 13, 2017

Uh oh!

JosephTremoulet commented Feb 13, 2017

Uh oh!

jkotas commented Feb 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikedn commented Feb 13, 2017

Uh oh!

JosephTremoulet commented Feb 13, 2017

Uh oh!

pgavlin commented Feb 13, 2017

Uh oh!

JosephTremoulet commented Feb 13, 2017

Uh oh!

AndyAyersMS commented Feb 13, 2017

Uh oh!

JosephTremoulet commented Feb 13, 2017

Uh oh!

AndyAyersMS commented Feb 13, 2017

Uh oh!

stephentoub commented Feb 13, 2017

Uh oh!

AndyAyersMS commented Feb 14, 2017

Uh oh!

JosephTremoulet commented Feb 14, 2017

Uh oh!

AndyAyersMS commented Feb 14, 2017

Uh oh!

AndyAyersMS commented Feb 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jkotas commented Feb 13, 2017 •

edited

Loading