Fix Issue 620 - Incorrect IND(ADDR(LCL_VAR)) folding when types do not match #40059
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11642,6 +11642,9 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) | |
| // TODO-1stClassStructs: improve this. | ||
| if (op1->IsLocal() || (op1->TypeGet() != TYP_STRUCT)) | ||
| { | ||
| // TODO-Cleanup: consider using GTF_IND_ASG_LHS here, as GTF_DONT_CSE here is used to indicate | ||
| // that this GT_IND is the LHS of an assignment | ||
| // | ||
| op1->gtFlags |= GTF_DONT_CSE; | ||
| } | ||
| break; | ||
|
|
@@ -13648,18 +13651,25 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) | |
| break; | ||
| } | ||
|
|
||
| bool foldAndReturnTemp; | ||
| bool isStore; | ||
| var_types addCastToType; | ||
|
|
||
| foldAndReturnTemp = false; | ||
| isStore = (tree->gtFlags & GTF_DONT_CSE) != 0; | ||
| ; | ||
| addCastToType = TYP_VOID; // TYP_VOID means we don't need to insert a cast | ||
|
|
||
| temp = nullptr; | ||
| ival1 = 0; | ||
|
|
||
| // Don't remove a volatile GT_IND, even if the address points to a local variable. | ||
| if ((tree->gtFlags & GTF_IND_VOLATILE) == 0) | ||
| { | ||
| /* Try to Fold *(&X) into X */ | ||
| if (op1->gtOper == GT_ADDR) | ||
| { | ||
| // Cannot remove a GT_ADDR if it is currently a CSE candidate. | ||
| if (gtIsActiveCSE_Candidate(op1)) | ||
| { | ||
| break; | ||
|
|
@@ -13678,8 +13688,9 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) | |
| } | ||
| else if (temp->OperIsLocal()) | ||
| { | ||
| unsigned lclNum = temp->AsLclVarCommon()->GetLclNum(); | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. I think that we can address any other change as a follow up. There was a problem hiding this comment. 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. There was a problem hiding this comment. I tried this change: But it results in a failure, to propagate a field sequence on x86: So that sequence is used by both GT_LCL_FLD and for GT_LCL_VAR that fail the optimization step. There was a problem hiding this comment. I ended up changing it to this: |
||
| LclVarDsc* varDsc = &lvaTable[lclNum]; | ||
| var_types lclType = varDsc->lvType; | ||
|
|
||
| // We will try to optimize when we have a promoted struct promoted with a zero lvFldOffset | ||
| if (varDsc->lvPromoted && (varDsc->lvFldOffset == 0)) | ||
|
|
@@ -13705,24 +13716,56 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) | |
| } | ||
| } | ||
| } | ||
| bool matchingTypes = (lclType == typ); | ||
briansull marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| bool matchingWidths = (genTypeSize(lclType) == genTypeSize(typ)); | ||
| bool matchingFloat = (varTypeIsFloating(lclType) == varTypeIsFloating(typ)); | ||
|
|
||
| // TYP_BOOL and TYP_UBYTE are also matching types | ||
| if (!matchingTypes) | ||
| { | ||
| if ((lclType == TYP_BOOL) && (typ == TYP_UBYTE)) | ||
| { | ||
| matchingTypes = true; | ||
| } | ||
| else if ((lclType == TYP_UBYTE) && (typ == TYP_BOOL)) | ||
| { | ||
| matchingTypes = true; | ||
| } | ||
| } | ||
|
|
||
| // If the sizes match and we don't have a float or TYP_STRUCT, | ||
| // then we will fold the IND/ADDR and use the LCL_VAR directly | ||
| // | ||
| if (matchingWidths && matchingFloat && (typ != TYP_STRUCT)) | ||
| { | ||
| // For some small types we might need to change to normalize loads or insert a cast here | ||
| // | ||
| if (varTypeIsSmall(typ)) | ||
| { | ||
| // For any stores of small types, we will force loads to be normalized | ||
| // this is necessary as we need to zero/sign extend any load | ||
| // after this kind of store. | ||
| // | ||
| if (isStore) | ||
| { | ||
| varDsc->lvForceLoadNormalize = true; | ||
| } | ||
| // otherwise we have a load operation | ||
| // | ||
| // Do we have a non matching small type load? | ||
| // if so we need to insert a cast operation | ||
| else if (!matchingTypes) | ||
| { | ||
| addCastToType = typ; | ||
| } | ||
| } | ||
| // we will fold the IND/ADDR and reduce to just the local variable | ||
briansull marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // | ||
| tree->gtType = typ = temp->TypeGet(); | ||
| foldAndReturnTemp = true; | ||
| } | ||
|
|
||
| if (!foldAndReturnTemp) | ||
briansull marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| // Assumes that when Lookup returns "false" it will leave "fieldSeq" unmodified (i.e. | ||
| // nullptr) | ||
|
|
@@ -13740,6 +13783,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) | |
| foldAndReturnTemp = true; | ||
| } | ||
| } | ||
|
|
||
| // Otherwise will will fold this into a GT_LCL_FLD below | ||
| // where we check (temp != nullptr) | ||
| } | ||
|
|
@@ -13912,19 +13956,42 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) | |
| // normalization. | ||
| if (temp->OperIs(GT_LCL_VAR)) | ||
| { | ||
| unsigned lclNum = temp->AsLclVarCommon()->GetLclNum(); | ||
briansull marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| var_types lclType = lvaTable[lclNum].lvType; | ||
|
|
||
| // 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); | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. 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.\ There was a problem hiding this comment. I totally agree that the |
||
|
|
||
| // Copy the GTF_DONT_CSE flag from the original tree to `result` | ||
| // because fgMorphLocalVar uses this flag to determine if we are loading or storing | ||
| // and will insert a cast when we are loading from a lvNormalizeOnLoad() local | ||
| // | ||
| result->gtFlags |= (tree->gtFlags & GTF_DONT_CSE); | ||
|
|
||
| // If the indirection node was a different small type than our local variable | ||
| // we insert a cast to that type. | ||
| // | ||
| if (addCastToType != TYP_VOID) | ||
| { | ||
| assert(varTypeIsSmall(addCastToType)); | ||
|
|
||
| // Insert a narrowing cast to the old indirection type | ||
| // | ||
| result = gtNewCastNode(TYP_INT, result, false, addCastToType); | ||
| } | ||
|
|
||
| // On this path we return 'result' after calling fgMorphTree on it. | ||
| // so now we can destroy the unused'temp' node. | ||
| // | ||
| DEBUG_DESTROY_NODE(temp); | ||
|
|
||
| temp = fgMorphTree(result); | ||
| } | ||
| else | ||
| { | ||
| assert(addCastToType == TYP_VOID); | ||
| } | ||
|
|
||
| return temp; | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
briansull marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| namespace NormalizeTest | ||
| { | ||
| class Program | ||
| { | ||
| static int testResult = 100; | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| static unsafe int Test1(short a, int b) | ||
| { | ||
| short c = (short)(a * 2); | ||
| int d = *((ushort*)&c) / b; | ||
| Console.WriteLine(d); | ||
| return d; | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| static unsafe short Test2(short a, int b) | ||
| { | ||
| short c = (short)(b * 2); | ||
| *((ushort*)&c) = (ushort)(a * b); | ||
| Console.WriteLine(c); | ||
| return c; | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| static unsafe int Test3(ushort a, int b) | ||
| { | ||
| ushort c = (ushort)(a * 2); | ||
| int d = *((short*)&c) / b; | ||
| Console.WriteLine(d); | ||
| return d; | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| static unsafe ushort Test4(ushort a, int b) | ||
| { | ||
| ushort c = (ushort)(b * 2); | ||
| *((short*)&c) = (short)(a * b); | ||
| Console.WriteLine(c); | ||
| return c; | ||
| } | ||
|
|
||
| struct S { | ||
| public short s16; | ||
| public ushort u16; | ||
| }; | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| static unsafe int TestF1(short a, int b) | ||
| { | ||
| S s; | ||
| s.s16 = (short)(a * 2); | ||
| int d = *((ushort*)&s.s16) / b; | ||
| Console.WriteLine(d); | ||
| return d; | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| static unsafe short TestF2(short a, int b) | ||
| { | ||
| S s; | ||
| s.s16 = (short)(b * 2); | ||
| *((ushort*)&s.s16) = (ushort)(a * b); | ||
| Console.WriteLine(s.s16); | ||
| return s.s16; | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| static unsafe int TestF3(ushort a, int b) | ||
| { | ||
| S s; | ||
| s.u16 = (ushort)(a * 2); | ||
| int d = *((short*)&s.u16) / b; | ||
| Console.WriteLine(d); | ||
| return d; | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| static unsafe ushort TestF4(ushort a, int b) | ||
| { | ||
| S s; | ||
| s.u16 = (ushort)(b * 2); | ||
| *((short*)&s.u16) = (short)(a * b); | ||
| Console.WriteLine(s.u16); | ||
| return s.u16; | ||
| } | ||
|
|
||
| static void Check(String id, int result, int expected) | ||
| { | ||
| if (result != expected) | ||
| { | ||
| Console.WriteLine("FAILED: {0} -- result {1}, expected {2}", id, result, expected); | ||
| testResult = -1; | ||
| } | ||
| } | ||
|
|
||
| static int Main() | ||
| { | ||
| int result1a = Test1(-1,1); | ||
| Check("Test1a", result1a, 65534); | ||
|
|
||
| int result1b = Test1(-1,-1); | ||
| Check("Test1b", result1b, -65534); | ||
|
|
||
| short result2a = Test2(-1,1); | ||
| Check("Test2a", (int) result2a, -1); | ||
|
|
||
| short result2b = Test2(-1,-1); | ||
| Check("Test2b", (int) result2b, 1); | ||
|
|
||
| int result3 = Test3(32767,-1); | ||
| Check("Test3", (int) result3, 2); | ||
|
|
||
| ushort result4 = Test4(32767,-1); | ||
| Check("Test4", (int) result4, 32769); | ||
|
|
||
|
|
||
|
|
||
| int resultF1a = TestF1(-1,1); | ||
| Check("TestF1a", resultF1a, 65534); | ||
|
|
||
| int resultF1b = TestF1(-1,-1); | ||
| Check("TestF1b", resultF1b, -65534); | ||
|
|
||
| short resultF2a = TestF2(-1,1); | ||
| Check("TestF2a", (int) resultF2a, -1); | ||
|
|
||
| short resultF2b = TestF2(-1,-1); | ||
| Check("TestF2b", (int) resultF2b, 1); | ||
|
|
||
| int resultF3 = TestF3(32767,-1); | ||
| Check("TestF3", (int) resultF3, 2); | ||
|
|
||
| ushort resultF4 = TestF4(32767,-1); | ||
| Check("TestF4", (int) resultF4, 32769); | ||
|
|
||
|
|
||
|
|
||
| if (testResult == 100) | ||
| { | ||
| Console.WriteLine("Test Passed"); | ||
| } | ||
| return testResult; | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <DebugType>None</DebugType> | ||
| <Optimize>True</Optimize> | ||
| <AllowUnsafeBlocks>True</AllowUnsafeBlocks> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <Compile Include="$(MSBuildProjectName).cs" /> | ||
| </ItemGroup> | ||
| </Project> |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like an unintentional new line with
;.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can fix this in a follow on checkin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there should be a comment here that this should be checking
GTF_IND_ASG_LHSonce the above cleanup item is addressed.