JIT: optimize more array covariant store checks in the importer by AndyAyersMS · Pull Request #189 · dotnet/runtime

AndyAyersMS · 2019-11-21T20:43:39Z

The importer was already optimizing away some array covariant store checks,
for cases where the value being stored was null, or the value being stored
came from the same array.

Change this to only optimize array covariant store checks in the importer
when optimization is enabled. For minopts, invoking the store helper produces
smaller code.

Update gtGetClassHandle to obtain the array handle from array newobjs,
and use this to also optimize cases where the destination array is exactly
object[] or is T[] where T is final and not itself subject to special
casting logic. In particular this gets the common case where T is string.

Closes dotnet/coreclr#6537.

The importer was already optimizing away some array covariant store checks, for cases where the value being stored was null, or the value being stored came from the same array. Change this to only optimize array covariant store checks in the importer when optimization is enabled. For minopts, invoking the store helper produces smaller code. Update `gtGetClassHandle` to obtain the array handle from array newobjs, and use this to also optimize cases where the destination array is exactly `object[]` or is `T[]` where `T` is final and not itself subject to special casting logic. In particular this gets the common case where `T` is `string`. Closes dotnet/coreclr#6537.

AndyAyersMS · 2019-11-21T20:46:53Z

Code size impact when optimizing (from dotnet/coreclr#6537).

Large regressions are all cases where loop cloning now kicks in, as this change makes more array accesses explicit in the JIT's IR, and that's what triggers cloning. Likely these are poor decisions by the cloner. Fixing that is a separate issue; see dotnet/coreclr#2634 and related.

Total bytes of diff: -27916 (-0.07% of base)
    diff is an improvement.

Top file regressions by size (bytes):
        1349 : System.Data.Common.dasm (0.09% of base)
         602 : System.Linq.Expressions.dasm (0.08% of base)
         201 : System.Collections.Specialized.dasm (0.79% of base)
         125 : System.Reflection.DispatchProxy.dasm (0.50% of base)
         106 : System.Collections.Concurrent.dasm (0.03% of base)

Top file improvements by size (bytes):
      -12978 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.41% of base)
       -6458 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.12% of base)
       -3051 : Microsoft.CodeAnalysis.CSharp.dasm (-0.07% of base)
       -1570 : System.Private.Xml.dasm (-0.04% of base)
        -725 : System.Net.HttpListener.dasm (-0.32% of base)

99 total files with size differences (82 improved, 17 regressed), 30 unchanged.

Top method regressions by size (bytes):
         576 (26.10% of base) : System.Linq.Expressions.dasm - StackSpiller:RewriteListInitExpression(ref,int):struct:this
         444 (13.12% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LocalRewriter:RewriteMultiDimensionalArrayForEachStatement(ref):ref:this
         322 ( 7.27% of base) : Microsoft.CodeAnalysis.CSharp.dasm - MethodBodySynthesizer:ConstructFieldLikeEventAccessorBody_Regular(ref,bool,ref,ref):ref
         322 (61.22% of base) : System.Private.CoreLib.dasm - MethodBase:CheckArguments(ref,ref,int,ref,ref):ref:this
         280 (17.41% of base) : System.Private.CoreLib.dasm - RuntimeType:MakeGenericType(ref):ref:this (2 methods)

Top method improvements by size (bytes):
       -1439 (-8.21% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Binder:ReportOverloadResolutionFailureForASingleCandidate(ref,ref,int,byref,struct,struct,bool,bool,bool,bool,ref,ref,bool,ref,ref):this
        -350 (-9.14% of base) : System.Private.CoreLib.dasm - CalendarData:CreateInvariant():ref (2 methods)
        -330 (-3.64% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - SyntaxFactory:GetNodeTypes():ref
        -305 (-0.95% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - VisualBasicCommandLineParser:Parse(ref,ref,ref,ref):ref:this
        -195 (-0.82% of base) : Microsoft.CodeAnalysis.CSharp.dasm - CSharpCommandLineParser:Parse(ref,ref,ref,ref):ref:this

Top method regressions by size (percentage):
         114 (83.82% of base) : System.Collections.Specialized.dasm - NameObjectCollectionBase:BaseGetAllKeys():ref:this
         114 (83.82% of base) : System.Collections.Specialized.dasm - NameObjectCollectionBase:BaseGetAllValues():ref:this
          75 (65.22% of base) : System.IO.FileSystem.DriveInfo.dasm - DriveInfo:GetDrives():ref
          90 (61.64% of base) : System.Data.Common.dasm - FieldNameLookup:.ctor(ref,int):this
         322 (61.22% of base) : System.Private.CoreLib.dasm - MethodBase:CheckArguments(ref,ref,int,ref,ref):ref:this

Top method improvements by size (percentage):
         -41 (-14.19% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - SynthesizedMyGroupCollectionPropertyGetAccessorSymbol:GetMethodBlock(ref,ref,ref):ref:this
         -41 (-14.19% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - SynthesizedMyGroupCollectionPropertySetAccessorSymbol:GetMethodBlock(ref,ref,ref):ref:this
         -37 (-13.17% of base) : System.Private.CoreLib.dasm - NativeRuntimeEventSource:MethodJitInliningSucceeded(ref,ref,ref,ref,ref,ref,ref,ref,ref,ushort):this
         -37 (-13.17% of base) : System.Private.CoreLib.dasm - NativeRuntimeEventSource:MethodJitInliningFailedAnsi(ref,ref,ref,ref,ref,ref,ref,ref,ref,bool):this
         -37 (-13.17% of base) : System.Private.CoreLib.dasm - NativeRuntimeEventSource:MethodJitTailCallFailedAnsi(ref,ref,ref,ref,ref,ref,ref,ref,ref,bool):this

4129 total methods with size differences (3799 improved, 330 regressed), 199577 unchanged

Will update with Tier0 code size impact when I have it.

AndyAyersMS · 2019-11-21T20:51:57Z

@dotnet/jit-contrib PTAL

erozenfeld

LGTM with a minor suggestion.

erozenfeld · 2019-11-22T00:09:40Z

src/coreclr/src/jit/importer.cpp

+                                {
+                                    DWORD elementAttribs = info.compCompHnd->getClassAttribs(arrayElementHandle);
+                                    DWORD flagsMask      = CORINFO_FLG_FINAL | CORINFO_FLG_MARSHAL_BYREF |
+                                                      CORINFO_FLG_CONTEXTFUL | CORINFO_FLG_VARIANCE | CORINFO_FLG_ARRAY;


This is the third place in the code where we have CORINFO_FLG_FINAL | CORINFO_FLG_MARSHAL_BYREF | CORINFO_FLG_CONTEXTFUL | CORINFO_FLG_VARIANCE as a mask (or part of a mask). Perhaps we should factor this into a helper.

Sure, will do.

AndyAyersMS · 2019-11-22T00:34:19Z

Tier0 diffs show a few regressions.

22 total methods with Code Size differences (16 improved, 6 regressed), 208571 unchanged.

The store null optimization happens in morph too, and we get slightly worse codegen in Tier0 if we transform the array store there as opposed to in the importer. So if we now defer in the importer at Tier0, it seems like we should defer in morph, too.

AndyAyersMS · 2019-11-22T01:21:22Z

Updated Tier0 diffs, no regressions.

PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies [tier0] for x64 default jit
Summary of Code Size diffs:
(Lower is better)
Total bytes of diff: -2147 (-0.00% of base)
    diff is an improvement.
Top file improvements (bytes):
       -1076 : System.Private.CoreLib.dasm (-0.02% of base)
        -217 : System.Private.Xml.dasm (-0.01% of base)
        -194 : System.Data.Common.dasm (-0.01% of base)
        -119 : System.Linq.Parallel.dasm (-0.01% of base)
         -92 : System.Runtime.Serialization.Formatters.dasm (-0.07% of base)
         -91 : System.Linq.Expressions.dasm (-0.01% of base)
         -62 : Microsoft.CodeAnalysis.dasm (-0.00% of base)
         -57 : System.ComponentModel.TypeConverter.dasm (-0.02% of base)
         -40 : System.Collections.NonGeneric.dasm (-0.12% of base)
         -31 : System.Private.DataContractSerialization.dasm (-0.00% of base)
         -27 : Microsoft.CodeAnalysis.CSharp.dasm (-0.00% of base)
         -24 : System.Collections.dasm (-0.00% of base)
         -21 : Microsoft.CSharp.dasm (-0.01% of base)
         -18 : Newtonsoft.Json.dasm (-0.00% of base)
         -18 : System.Private.Xml.Linq.dasm (-0.01% of base)
         -17 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.00% of base)
         -15 : System.Collections.Immutable.dasm (-0.00% of base)
         -14 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.00% of base)
         -14 : System.Transactions.Local.dasm (-0.01% of base)
19 total files with Code Size differences (19 improved, 0 regressed), 110 unchanged.
Top method improvements (bytes):
        -138 (-10.86% of base) : System.Private.CoreLib.dasm - DefaultBinder:BindToField(int,ref,Object,CultureInfo):FieldInfo:this
        -118 (-1.49% of base) : System.Private.CoreLib.dasm - DefaultBinder:BindToMethod(int,ref,byref,ref,CultureInfo,ref,byref):MethodBase:this
        -102 (-4.92% of base) : System.Private.CoreLib.dasm - Type:FindMembers(int,int,MemberFilter,Object):ref:this
        -101 (-9.38% of base) : System.Private.Xml.dasm - XsltCompileContext:FindBestMethod(ref,bool,bool,String,ref):MethodInfo:this
         -96 (-1.17% of base) : System.Private.CoreLib.dasm - TlsOverPerCoreLockedStacksArrayPool`1:Rent(int):ref:this (8 methods)
         -91 (-2.09% of base) : System.Private.CoreLib.dasm - WhenAllPromise`1:Invoke(Task):this (7 methods)
         -84 (-1.99% of base) : System.Private.CoreLib.dasm - LockedStack:Trim(int,int,int,int):this (7 methods)
         -63 (-9.00% of base) : System.Data.Common.dasm - RBTree`1:FreePage(TreePage):this (7 methods)
         -63 (-6.08% of base) : System.Linq.Parallel.dasm - OrderedPipeliningMergeEnumerator:TryGetPrivateElement(int,byref):bool:this (7 methods)
         -56 (-4.19% of base) : System.Linq.Parallel.dasm - AsynchronousChannel`1:InternalDequeueChunk():ref:this (7 methods)
         -56 (-1.82% of base) : System.Private.CoreLib.dasm - Bucket:Rent():ref:this (7 methods)
         -56 (-4.82% of base) : System.Private.CoreLib.dasm - LockedStack:TryPop():ref:this (7 methods)
         -53 (-22.27% of base) : System.Linq.Expressions.dasm - CallSite`1:MoveRule(int):this
         -49 (-6.65% of base) : System.Private.CoreLib.dasm - Type:GetEnumData(byref,byref):this
         -38 (-1.10% of base) : System.Private.CoreLib.dasm - DefaultBinder:SelectMethod(int,ref,ref,ref):MethodBase:this (2 methods)
         -33 (-6.25% of base) : System.Private.CoreLib.dasm - WorkStealingQueue:LocalPopCore():Object:this
         -30 (-1.64% of base) : Microsoft.CodeAnalysis.dasm - ImmutableArrayExtensions:DeOrder(ImmutableArray`1):ImmutableArray`1 (7 methods)
         -30 (-5.50% of base) : System.Private.Xml.dasm - XmlQueryNodeSequence:DocOrderDistinct(IComparer`1):XmlQueryNodeSequence:this
         -27 (-6.78% of base) : System.Private.CoreLib.dasm - DateTimeFormatInfo:GetMergedPatterns(ref,String):ref
         -26 (-1.32% of base) : System.Runtime.Serialization.Formatters.dasm - ObjectManager:CompleteObject(ObjectHolder,bool):this
Top method improvements (percentages):
         -15 (-22.39% of base) : System.Linq.Expressions.dasm - Reference:Run(InterpretedFrame):int:this
         -53 (-22.27% of base) : System.Linq.Expressions.dasm - CallSite`1:MoveRule(int):this
         -14 (-21.54% of base) : Microsoft.CodeAnalysis.CSharp.dasm - SyntaxNodeOrTokenListBuilder:RemoveLast():this
         -14 (-21.54% of base) : System.Transactions.Local.dasm - Bucket:Remove(InternalTransaction):this
         -15 (-15.79% of base) : System.Data.Common.dasm - SchemaMapping:LoadDataRowWithClear():this
         -14 (-13.86% of base) : System.Private.Xml.dasm - XmlSortKeyAccumulator:Create():this
         -14 (-12.96% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - SyntaxNodeOrTokenListBuilder:RemoveLast():this
        -138 (-10.86% of base) : System.Private.CoreLib.dasm - DefaultBinder:BindToField(int,ref,Object,CultureInfo):FieldInfo:this
         -14 (-9.86% of base) : System.Data.Common.dasm - StringStorage:Set(int,Object):this
        -101 (-9.38% of base) : System.Private.Xml.dasm - XsltCompileContext:FindBestMethod(ref,bool,bool,String,ref):MethodInfo:this
         -23 (-9.02% of base) : System.Linq.Expressions.dasm - ChildRewriter:Add(Expression):this
         -63 (-9.00% of base) : System.Data.Common.dasm - RBTree`1:FreePage(TreePage):this (7 methods)
         -14 (-8.75% of base) : System.Private.Xml.dasm - XmlSortKeyAccumulator:FinishSortKeys():this
         -17 (-7.94% of base) : System.Data.Common.dasm - SchemaMapping:FreeDataRowChapters():this
         -24 (-7.12% of base) : System.Collections.NonGeneric.dasm - SortedList:RemoveAt(int):this
         -27 (-6.78% of base) : System.Private.CoreLib.dasm - DateTimeFormatInfo:GetMergedPatterns(ref,String):ref
         -49 (-6.65% of base) : System.Private.CoreLib.dasm - Type:GetEnumData(byref,byref):this
         -13 (-6.47% of base) : System.Private.Xml.dasm - QilList:RemoveAt(int):this
         -17 (-6.25% of base) : System.Private.CoreLib.dasm - ILGenerator:SortExceptions(ref)
         -33 (-6.25% of base) : System.Private.CoreLib.dasm - WorkStealingQueue:LocalPopCore():Object:this
82 total methods with Code Size differences (82 improved, 0 regressed), 208511 unchanged.

jkotas · 2019-11-22T04:48:12Z

src/coreclr/src/jit/importer.cpp

-                    if (indexFromOp1->OperGet() == GT_LCL_VAR)
+                    // Check for assignment to same array, ie. arrLcl[i] = arrLcl[j]
+                    // This does not need CORINFO_HELP_ARRADDR_ST
+                    if (arrayNodeFrom->OperGet() == GT_INDEX && arrayNodeTo->gtOper == GT_LCL_VAR)


Factor all this logic into a separate method? E.g. impCanSkipCovariantStoreCheck ?

Good idea, will do.

jkotas · 2019-11-22T12:06:17Z

src/coreclr/src/jit/importer.cpp

+//
+bool Compiler::impCanSkipCovariantStoreCheck(GenTree* arrayNodeFrom, GenTree* arrayNodeTo)
+{
+    if (!opts.OptimizationEnabled())


Nit: Micro-optimize for fast JIT and move this check to be at the callsite of this method?

I have looked at other similar places in the JIT that call OptimizationEnabled(). Some of them have at the callsite and some of them have as the early out like this.

In this case I think it reads better having it at the call site, and there's just one call site, so I'll move it there.

jkotas · 2019-11-22T17:40:37Z

Build breaks...

erozenfeld

LGTM with a few nits.

erozenfeld · 2019-11-22T18:41:41Z

src/coreclr/src/jit/importer.cpp

+        return true;
+    }
+
+    // Check if destination array is exactly object[].


This comment is now misplaced.

erozenfeld · 2019-11-22T18:43:06Z

src/coreclr/src/jit/importer.cpp

+    //
+    // Note we are conservative on array of arrays here. It might be worth checking
+    // for element types like int[].
+    const bool elementTypeIsExact = impIsClassExact(arrayElementHandle);


The local variable seems unnecessary since it's used just one.

erozenfeld · 2019-11-22T18:44:55Z

src/coreclr/src/jit/importer.cpp

-                DWORD flags = info.compCompHnd->getClassAttribs(pResolvedToken->hClass);
-
                // If the class is final and is not marshal byref, variant or
                // contextful, the jit can expand the IsInst check inline.


I would remove or rephrase this comment since it can easily get out of sync with the implementation of impIsClassExact.

erozenfeld · 2019-11-22T18:47:25Z

src/coreclr/src/jit/importer.cpp

+//    can skip the array store covariance check.
+//
+// Arguments:
+//    arrayNodeFrom -- tree producing the value to store


This name is slightly confusing since the value to store isn't normally an array. Maybe simply nodeFrom?

How about just value and array.

Sounds good to me.

AndyAyersMS · 2019-11-22T21:46:59Z

@erozenfeld last commit should address your feedback

erozenfeld · 2019-11-22T22:57:15Z

LGTM, thanks

Removes Object and UIntPtr as valid cast from pointer. Fixes the following code which returned true and now returns false: ``` typeof(object).IsAssignableFrom(typeof(byte*)) typeof(UIntPtr).IsAssignableFrom(typeof(byte*)) ``` Fixes dotnet#189

Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 21, 2019

erozenfeld approved these changes Nov 22, 2019

View reviewed changes

AndyAyersMS added 2 commits November 21, 2019 17:10

defer null store opt in morph too

674c075

review feedback

4913828

jkotas reviewed Nov 22, 2019

View reviewed changes

review feedback 2

1175daa

jkotas reviewed Nov 22, 2019

View reviewed changes

review feedback 3

d512117

jkotas approved these changes Nov 22, 2019

View reviewed changes

fix issues with last commit

a015783

erozenfeld approved these changes Nov 22, 2019

View reviewed changes

review feedback 4

f325bf4

AndyAyersMS merged commit 05aadb8 into dotnet:master Nov 25, 2019

AndyAyersMS deleted the CovariantStoreImporterOpts branch November 25, 2019 16:20

AndyAyersMS added tenet-performance Performance related issue optimization labels Mar 7, 2020

karelz added this to the 5.0.0 milestone Aug 18, 2020

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Conversation

AndyAyersMS commented Nov 21, 2019

Uh oh!

AndyAyersMS commented Nov 21, 2019

Uh oh!

AndyAyersMS commented Nov 21, 2019

Uh oh!

erozenfeld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

AndyAyersMS commented Nov 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Nov 22, 2019

Uh oh!

erozenfeld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Nov 22, 2019

Uh oh!

erozenfeld commented Nov 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AndyAyersMS commented Nov 22, 2019 •

edited

Loading