Fix Issue 620 - Incorrect IND(ADDR(LCL_VAR)) folding when types do not match #40059

briansull · 2020-07-29T03:59:23Z

Fixes for morphing a GT_IND of a GT_ADDR of a GT_LCL_VAR
When the GT_IND size is a small type.
We can have a GT_IND as either a load or as a store,
it is a store when it is on the Left-Hand-Side of an assignment
Otherwise it is a load

The test case provide with this PR covers the various combinations that were incorrect.

briansull · 2020-07-29T20:38:41Z

Fixes #620

briansull · 2020-07-30T01:59:51Z

Diff is an improvement

Crossgen CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies for x64 default jit
164 total methods with Code Size differences (118 improved, 46 regressed), 227498 unchanged.

Top file regressions (bytes):
          45 : Microsoft.CodeAnalysis.CSharp.dasm (0.00% of base)
          15 : Microsoft.CodeAnalysis.VisualBasic.dasm (0.00% of base)
          14 : System.Text.Json.dasm (0.00% of base)
           8 : System.Private.Xml.dasm (0.00% of base)
           6 : Microsoft.Extensions.Primitives.dasm (0.04% of base)
           4 : Microsoft.CodeAnalysis.dasm (0.00% of base)
           2 : System.Reflection.Metadata.dasm (0.00% of base)
           2 : System.Security.Cryptography.Algorithms.dasm (0.00% of base)
           2 : System.Security.Cryptography.Cng.dasm (0.00% of base)
           2 : System.Security.Cryptography.Pkcs.dasm (0.00% of base)
           1 : System.Net.HttpListener.dasm (0.00% of base)
           1 : System.Transactions.Local.dasm (0.00% of base)
Top file improvements (bytes):
       -1025 : System.Private.CoreLib.dasm (-0.03% of base)
         -15 : System.Data.Odbc.dasm (-0.01% of base)
         -10 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.00% of base)
          -3 : System.Diagnostics.PerformanceCounter.dasm (-0.00% of base)

Top method regressions (bytes):
          11 ( 1.36% of base) : System.Private.CoreLib.dasm - System.Reflection.MdConstant:GetValue(System.Reflection.MetadataImport,int,System.RuntimeTypeHandle,bool):System.Object
          10 ( 1.77% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.VisualBasicCommandLineParser:PublicSymbolsToInternalDefines(System.Collections.Generic.IEnumerable`1[[System.Collections.Generic.KeyValuePair`2[[System.String, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Object, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]],System.String):System.Collections.Immutable.ImmutableDictionary`2[[System.String, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax.CConst, Microsoft.CodeAnalysis.VisualBasic, Version=1.1.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35]]
           8 ( 1.86% of base) : System.Private.CoreLib.dasm - System.Diagnostics.Tracing.ManifestBuilder:AddChannel(System.String,int,System.Diagnostics.Tracing.EventChannelAttribute):this
           8 ( 0.58% of base) : System.Private.Xml.dasm - System.Xml.XmlTextReaderImpl:ParseNumericCharRefInline(int,bool,System.Text.StringBuilder,byref,byref):int:this

Top method improvements (bytes):
         -69 (-16.01% of base) : System.Private.CoreLib.dasm - System.SpanHelpers:LastIndexOf(byref,ushort,int):int
         -61 (-14.39% of base) : System.Private.CoreLib.dasm - System.SpanHelpers:IndexOf(byref,ushort,int):int
         -40 (-57.97% of base) : System.Private.CoreLib.dasm - System.MemoryExtensions:IndexOfAny(System.ReadOnlySpan`1[Char],ushort,ushort,ushort):int
         -36 (-2.04% of base) : System.Private.CoreLib.dasm - System.Threading.Tasks.Task:RunContinuations(System.Object):this
         -26 (-50.00% of base) : System.Private.CoreLib.dasm - System.MemoryExtensions:IndexOfAny(System.ReadOnlySpan`1[Char],ushort,ushort):int
         -17 (-25.76% of base) : System.Private.CoreLib.dasm - System.Buffers.Binary.BinaryPrimitives:WriteInt16BigEndian(System.Span`1[Byte],short)
         -17 (-30.36% of base) : System.Private.CoreLib.dasm - System.Buffers.Binary.BinaryPrimitives:TryWriteInt16BigEndian(System.Span`1[Byte],short):bool
         -16 (-25.40% of base) : System.Private.CoreLib.dasm - System.Buffers.Binary.BinaryPrimitives:WriteUInt16BigEndian(System.Span`1[Byte],ushort)
         -16 (-30.19% of base) : System.Private.CoreLib.dasm - System.Buffers.Binary.BinaryPrimitives:TryWriteUInt16BigEndian(System.Span`1[Byte],ushort):bool
         -14 (-6.51% of base) : System.Private.CoreLib.dasm - System.Text.StringBuilder:AppendSpanFormattable(short):System.Text.StringBuilder:this
         -14 (-6.51% of base) : System.Private.CoreLib.dasm - System.Text.StringBuilder:AppendSpanFormattable(byte):System.Text.StringBuilder:this
         -13 (-3.76% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.Etlx.TraceActivity:ActivityKindToString(short):System.String
         -12 (-6.63% of base) : System.Private.CoreLib.dasm - System.Text.StringBuilder:AppendSpanFormattable(ushort):System.Text.StringBuilder:this
         -12 (-6.63% of base) : System.Private.CoreLib.dasm - System.Text.StringBuilder:AppendSpanFormattable(ubyte):System.Text.StringBuilder:this
         -12 (-29.27% of base) : System.Private.CoreLib.dasm - System.Collections.Generic.GenericComparer`1[Boolean][System.Boolean]:Compare(bool,bool):int:this
         -12 (-60.00% of base) : System.Private.CoreLib.dasm - System.Collections.Generic.GenericEqualityComparer`1[Char][System.Char]:GetHashCode(ushort):int:this

Add check for loads using GTF_IND_ASG_LHS as we can read and normalize loads using a small type, but not stores. For stores will will force the use of a GT_LCL_FLD whewn using small types

briansull · 2020-08-04T20:36:05Z

@dotnet/jit-contrib PTAL

sandreenko

There is a concern about GT_LCL_FLD support.

src/coreclr/src/jit/morph.cpp

src/tests/JIT/Regression/JitBlue/Runtime_620/Runtime_620.cs

src/coreclr/src/jit/morph.cpp

briansull · 2020-08-06T04:09:46Z

@sandreenko PTAL

briansull · 2020-08-06T17:19:26Z

dotnet-runtime-perf (Libraries Build Linux x64 Release) is currently broken:
APICompat failures on Linux #40416

sandreenko

A few questions and could you please trigger outerloop/stress job for this PR before merge?

sandreenko · 2020-08-06T18:35:35Z

src/coreclr/src/jit/morph.cpp

-            temp              = nullptr;
-            ival1             = 0;
+            isStore           = (tree->gtFlags & GTF_DONT_CSE) != 0;
+            ;


looks like an unintentional new line with ;.

I can fix this in a follow on checkin

Also, there should be a comment here that this should be checking GTF_IND_ASG_LHS once the above cleanup item is addressed.

sandreenko · 2020-08-06T19:00:15Z

src/coreclr/src/jit/morph.cpp

@@ -13678,8 +13688,9 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
                    }
                    else if (temp->OperIsLocal())
                    {
-                        unsigned   lclNum = temp->AsLclVarCommon()->GetLclNum();
-                        LclVarDsc* varDsc = &lvaTable[lclNum];
+                        unsigned   lclNum  = temp->AsLclVarCommon()->GetLclNum();


It is still confusing that we are checking parent struct type for local fields even if these checks never pass and the code is working because of that.

I would suggest separate this else if (temp->OperIsLocal())
as

else if (temp->OperIs(GT_LCL_VAR)) { the new logic} else if (temp->OperIs(GT_LCL_FLD)) // the old logic that was applying to `LCL_FLD` only. { // Assumes that when Lookup returns "false" it will leave "fieldSeq" unmodified (i.e. // nullptr) assert(fieldSeq == nullptr); bool b = GetZeroOffsetFieldMap()->Lookup(op1, &fieldSeq); assert(b || fieldSeq == nullptr); if (fieldSeq != nullptr) // that is from the initial CoreCLR commit, no idea why we do it only // when we have a zero offset. { // Append the field sequence, change the type. temp->AsLclFld()->SetFieldSeq( GetFieldSeqStore()->Append(temp->AsLclFld()->GetFieldSeq(), fieldSeq)); temp->gtType = typ; foldAndReturnTemp = true; } }

I think that we can address any other change as a follow up.

I'm not sure why there is a rush to check this in today; I think it's worth making the change @sandreenko suggests, as it doesn't seem reasonable to use the parent type in the `GT_LCL_FLD' case.

I tried this change:

else if (temp->OperIs(GT_LCL_FLD)) // the old logic that was applying to `LCL_FLD` only. else if (temp->OperIs(GT_LCL_VAR)) { the new logic}

But it results in a failure, to propagate a field sequence on x86:

fgMorphTree BB04, STMT00037 (before) [000221] -A---------- * ASG struct (copy) [000220] ------------ +--* BLK struct<System.DateTime, 8> [000219] ------------ | \--* ADDR byref [000218] -------N---- | \--* FIELD struct Start [000215] ------------ | \--* ADDR byref [000216] -------N---- | \--* LCL_VAR struct<System.Globalization.DaylightTimeStruct, 24> V11 tmp2 [000217] ------------ \--* LCL_VAR struct<System.DateTime, 8>(P) V24 tmp15 \--* long V24._dateData (offs=0x00) -> V42 tmp33 fgMorphTree BB04, STMT00037 (after) [000355] -A---+------ * ASG long [000353] *------N---- +--* IND long [000351] ------------ | \--* ADDR byref < [000352] U------N---- | \--* LCL_FLD struct V11 tmp2 [+0] Fseq[Start, _dateData] > [000352] U------N---- | \--* LCL_FLD struct V11 tmp2 [+0] [000354] ------------ \--* LCL_VAR long V42 tmp33

So that sequence is used by both GT_LCL_FLD and for GT_LCL_VAR that fail the optimization step.

I ended up changing it to this:

else if (temp->OperIsLocal()) { if (temp->OperIs(GT_LCL_VAR)) { // newer code } if (!foldAndReturnTemp) { // old code

src/coreclr/src/jit/morph.cpp

briansull · 2020-08-06T19:50:35Z

I will trigger an outerloop/stress job for this PR before merge...

I have already run a JitStress=1 run on my system, so I don't expect any failures caused by my change,
Do you know if out outerloop Jit Stress is clean as of now?

I would ask that you sign off NOW so I can check in immediately after it finishes.

briansull · 2020-08-06T19:55:24Z

FYI: JitStress outerloop has failed 9 of the last 9 runs,

Pipeline Insights

This pipeline has a pass rate of 28.57% ( 50%) in the last 14 days.

briansull · 2020-08-06T19:58:12Z

Jit Stress outerloop tests now have started,

I am not going to review or approve this change.

briansull · 2020-08-06T20:44:49Z

@CarolEidt - have you analyzed the regressions? Are they expected? Are some of them GT_LCL_FLD cases?

Yes, I have spend a lot of time doing analysis on the differences and insuring that we don't have regressions.
I will post a sample diff for both a regression and an improvement later on this afternoon.

We typically don't see this pattern with a GT_LCL_FLD during the first morph phase, we only see them when we do a late remorph due to an assertionprop or a CSE subsitution.

CarolEidt

I think it's worth being conservative here, and it makes sense to preserve the existing code for the GT_LCL_FLD case.

CarolEidt · 2020-08-06T20:39:45Z

src/coreclr/src/jit/morph.cpp

@@ -13678,8 +13688,9 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
                    }
                    else if (temp->OperIsLocal())
                    {
-                        unsigned   lclNum = temp->AsLclVarCommon()->GetLclNum();
-                        LclVarDsc* varDsc = &lvaTable[lclNum];
+                        unsigned   lclNum  = temp->AsLclVarCommon()->GetLclNum();


I'm not sure why there is a rush to check this in today; I think it's worth making the change @sandreenko suggests, as it doesn't seem reasonable to use the parent type in the `GT_LCL_FLD' case.

CarolEidt · 2020-08-06T20:43:05Z

src/coreclr/src/jit/morph.cpp

-            temp              = nullptr;
-            ival1             = 0;
+            isStore           = (tree->gtFlags & GTF_DONT_CSE) != 0;
+            ;


Also, there should be a comment here that this should be checking GTF_IND_ASG_LHS once the above cleanup item is addressed.

src/coreclr/src/jit/morph.cpp

CarolEidt · 2020-08-06T20:50:28Z

src/coreclr/src/jit/morph.cpp

+                    // We build a new 'result' tree to return, as we want to call fgMorphTree on it
+                    //
+                    GenTree* result = gtNewLclvNode(lclNum, lclType);
+                    assert(result->OperGet() == GT_LCL_VAR);


Why is it that we need to build a new result tree? It seems an unfortunate additional cost; why doesn't it work to clearn the GTF_DEBUG_NODE_MORPHED flag?

The whole clearing/setting the GTF_DEBUG_NODE_MORPHED flag is a total hack.

But fine I will revert the change to my original fix which is much simpler but introduces some regressions.

I believe that this fix is better, but will go with whatever you want.\

I totally agree that the GTF_DEBUG_NODE_MOPRHED flag is a frustratingly fragile mechanism - though I do think it would be best to do a minimal fix for now.

briansull · 2020-08-07T16:20:53Z

Replaced with #40535

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

briansull force-pushed the issue_620 branch from f68745b to de025b6 Compare July 29, 2020 18:15

jkoritzinsky linked an issue Jul 30, 2020 that may be closed by this pull request

Incorrect IND(ADDR(LCL_VAR)) folding when types do not match #620

Closed

briansull force-pushed the issue_620 branch from de025b6 to 23e60cd Compare August 4, 2020 04:56

briansull added 5 commits August 4, 2020 09:56

Added isLoad and matchingType

3fd5254

Fix for Issue 620

917729e

Add check for loads using GTF_IND_ASG_LHS as we can read and normalize loads using a small type, but not stores. For stores will will force the use of a GT_LCL_FLD whewn using small types

Added test case for issue 620

899d78a

added lvForceLoadNormalize

b9aa946

Improved the fix and now it is an code size improvement

928bbee

briansull self-assigned this Aug 4, 2020

briansull force-pushed the issue_620 branch from 370727d to 928bbee Compare August 4, 2020 20:09

briansull requested a review from BruceForstall August 4, 2020 20:36

Always insert a cast for loads when the indirection type is different

fd266c8

briansull requested a review from sandreenko August 5, 2020 02:15

sandreenko previously requested changes Aug 5, 2020

View reviewed changes

briansull added 2 commits August 5, 2020 17:48

Code review feedback

32df3b6

Added additional test cases for short unpromoted struct fields

e4cf61b

sandreenko reviewed Aug 6, 2020

View reviewed changes

CarolEidt suggested changes Aug 6, 2020

View reviewed changes

dotnet deleted a comment from CarolEidt Aug 7, 2020

briansull added 2 commits August 6, 2020 20:08

Codereview suggestions, Expanded test case

cf09924

In morph mark the left hand side of an assignment with GTF_IND_ASG_LHS

720555a

briansull closed this Aug 7, 2020

karelz added this to the 5.0.0 milestone Aug 18, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue 620 - Incorrect IND(ADDR(LCL_VAR)) folding when types do not match #40059

Fix Issue 620 - Incorrect IND(ADDR(LCL_VAR)) folding when types do not match #40059

briansull commented Jul 29, 2020 •

edited

Loading

briansull commented Jul 29, 2020

briansull commented Jul 30, 2020 •

edited

Loading

briansull commented Aug 4, 2020

sandreenko left a comment

briansull commented Aug 6, 2020

briansull commented Aug 6, 2020

sandreenko left a comment

sandreenko Aug 6, 2020

briansull Aug 6, 2020 •

edited

Loading

CarolEidt Aug 6, 2020

sandreenko Aug 6, 2020

briansull Aug 6, 2020 •

edited

Loading

CarolEidt Aug 6, 2020

briansull Aug 7, 2020 •

edited

Loading

briansull Aug 7, 2020 •

edited

Loading

briansull commented Aug 6, 2020

briansull commented Aug 6, 2020

briansull commented Aug 6, 2020

briansull commented Aug 6, 2020

CarolEidt left a comment

CarolEidt Aug 6, 2020

CarolEidt Aug 6, 2020

CarolEidt Aug 6, 2020

briansull Aug 6, 2020

CarolEidt Aug 6, 2020

briansull commented Aug 7, 2020

Fix Issue 620 - Incorrect IND(ADDR(LCL_VAR)) folding when types do not match #40059

Fix Issue 620 - Incorrect IND(ADDR(LCL_VAR)) folding when types do not match #40059

Conversation

briansull commented Jul 29, 2020 • edited Loading

briansull commented Jul 29, 2020

briansull commented Jul 30, 2020 • edited Loading

briansull commented Aug 4, 2020

sandreenko left a comment

Choose a reason for hiding this comment

briansull commented Aug 6, 2020

briansull commented Aug 6, 2020

sandreenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansull Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansull Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansull Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

briansull Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

briansull commented Aug 6, 2020

briansull commented Aug 6, 2020

briansull commented Aug 6, 2020

briansull commented Aug 6, 2020

CarolEidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansull commented Aug 7, 2020

briansull commented Jul 29, 2020 •

edited

Loading

briansull commented Jul 30, 2020 •

edited

Loading

briansull Aug 6, 2020 •

edited

Loading

briansull Aug 6, 2020 •

edited

Loading

briansull Aug 7, 2020 •

edited

Loading

briansull Aug 7, 2020 •

edited

Loading