Fix an incorrect CSE case with struct retyping. (#34676)

* Fix old printing issues.

* add a repro test that doesn't require crossgen2.

* Check that all `ADDR` sources are marked as DONT_CSE.

* Fix text(no size) diffs found by SPMI.

* Add a question/comment.

* Update src/coreclr/src/jit/compiler.hpp

Co-Authored-By: Eugene Rozenfeld <erozen@microsoft.com>
* Delete an optimization that was not proved to be correct.

* Delete useless code.

* Revert "Fix text(no size) diffs found by SPMI."

This reverts commit cd24383210b9ed22f7ea3640005217021f9fbd63.

diff --git a/src/coreclr/src/jit/compiler.hpp b/src/coreclr/src/jit/compiler.hpp
index a3b3838..c0b2023 100644 (file)
--- a/src/coreclr/src/jit/compiler.hpp
+++ b/src/coreclr/src/jit/compiler.hpp
@@ -969,9 +969,11 @@ inline GenTree* Compiler::gtNewOperNode(genTreeOps oper, var_types type, GenTree
             // IND(ADDR(IND(x)) == IND(x)
             if (op1->gtOper == GT_ADDR)
             {
-                if (op1->AsOp()->gtOp1->gtOper == GT_IND && (op1->AsOp()->gtOp1->gtFlags & GTF_IND_ARR_INDEX) == 0)
+                GenTreeUnOp* addr  = op1->AsUnOp();
+                GenTree*     indir = addr->gtGetOp1();
+                if (indir->OperIs(GT_IND) && ((indir->gtFlags & GTF_IND_ARR_INDEX) == 0))
                 {
-                    op1 = op1->AsOp()->gtOp1->AsOp()->gtOp1;
+                    op1 = indir->AsIndir()->Addr();
                 }
             }
         }
@@ -982,6 +984,11 @@ inline GenTree* Compiler::gtNewOperNode(genTreeOps oper, var_types type, GenTree
             {
                 return op1->AsOp()->gtOp1;
             }
+            else
+            {
+                // Addr source can't be CSE-ed.
+                op1->SetDoNotCSE();
+            }
         }
     }
 

diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp
index e2c2722..b86d5fb 100644 (file)
--- a/src/coreclr/src/jit/flowgraph.cpp
+++ b/src/coreclr/src/jit/flowgraph.cpp
@@ -21225,6 +21225,8 @@ void Compiler::fgDebugCheckFlags(GenTree* tree)
                     return;
                 }
                 break;
+            case GT_ADDR:
+                assert(!op1->CanCSE());
 
             default:
                 break;

diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp
index f5b72fc..6434c44 100644 (file)
--- a/src/coreclr/src/jit/gentree.cpp
+++ b/src/coreclr/src/jit/gentree.cpp
@@ -11285,6 +11285,8 @@ void Compiler::gtDispTree(GenTree*     tree,
                 printf(" %s", eeGetFieldName(tree->AsField()->gtFldHnd), 0);
             }
 
+            gtDispCommonEndLine(tree);
+
             if (tree->AsField()->gtFldObj && !topOnly)
             {
                 gtDispVN(tree);

diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h
index ad1e1e1..83603a1 100644 (file)
--- a/src/coreclr/src/jit/gentree.h
+++ b/src/coreclr/src/jit/gentree.h
@@ -1941,6 +1941,21 @@ public:
         ClearRegOptional();
     }
 
+    bool CanCSE() const
+    {
+        return ((gtFlags & GTF_DONT_CSE) == 0);
+    }
+
+    void SetDoNotCSE()
+    {
+        gtFlags |= GTF_DONT_CSE;
+    }
+
+    void ClearDoNotCSE()
+    {
+        gtFlags &= ~GTF_DONT_CSE;
+    }
+
     bool IsReverseOp() const
     {
         return (gtFlags & GTF_REVERSE_OPS) ? true : false;

diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp
index fac4819..20703fa 100644 (file)
--- a/src/coreclr/src/jit/morph.cpp
+++ b/src/coreclr/src/jit/morph.cpp
@@ -5376,7 +5376,12 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree)
         tree->ChangeOper(GT_IND);
         GenTreeIndir* const indir = tree->AsIndir();
         indir->Addr()             = indexAddr;
+        bool canCSE               = indir->CanCSE();
         indir->gtFlags            = GTF_IND_ARR_INDEX | (indexAddr->gtFlags & GTF_ALL_EFFECT);
+        if (!canCSE)
+        {
+            indir->SetDoNotCSE();
+        }
 
 #ifdef DEBUG
         indexAddr->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED;
@@ -13384,9 +13389,6 @@ DONE_MORPHING_CHILDREN:
 
                 return tree;
             }
-
-            /* op1 of a GT_ADDR is an l-value. Only r-values can be CSEed */
-            op1->gtFlags |= GTF_DONT_CSE;
             break;
 
         case GT_COLON:
@@ -16831,8 +16833,7 @@ void Compiler::fgMorphStructField(GenTree* tree, GenTree* parent)
                 tree->SetOper(GT_LCL_VAR);
                 tree->AsLclVarCommon()->SetLclNum(fieldLclIndex);
                 tree->gtType = fieldType;
-                tree->gtFlags &= GTF_NODE_MASK;
-                tree->gtFlags &= ~GTF_GLOB_REF;
+                tree->gtFlags &= GTF_NODE_MASK; // Note: that clears all flags except `GTF_COLON_COND`.
 
                 if (parent->gtOper == GT_ASG)
                 {
@@ -17676,7 +17677,7 @@ bool Compiler::fgMorphCombineSIMDFieldAssignments(BasicBlock* block, Statement*
     if (verbose)
     {
         printf("\nFound contiguous assignments from a SIMD vector to memory.\n");
-        printf("From " FMT_BB ", stmt", block->bbNum);
+        printf("From " FMT_BB ", stmt ", block->bbNum);
         printStmtID(stmt);
         printf(" to stmt");
         printStmtID(lastStmt);
@@ -17742,13 +17743,16 @@ bool Compiler::fgMorphCombineSIMDFieldAssignments(BasicBlock* block, Statement*
 #ifdef DEBUG
     if (verbose)
     {
-        printf("\n" FMT_BB " stmt", block->bbNum);
+        printf("\n" FMT_BB " stmt ", block->bbNum);
         printStmtID(stmt);
         printf("(before)\n");
         gtDispStmt(stmt);
     }
 #endif
 
+    assert(!simdStructNode->CanCSE());
+    simdStructNode->ClearDoNotCSE();
+
     tree = gtNewAssignNode(dstNode, simdStructNode);
 
     stmt->SetRootNode(tree);

diff --git a/src/coreclr/src/jit/valuenum.cpp b/src/coreclr/src/jit/valuenum.cpp
index f4060cb..2fb9c3c 100644 (file)
--- a/src/coreclr/src/jit/valuenum.cpp
+++ b/src/coreclr/src/jit/valuenum.cpp
@@ -3612,15 +3612,7 @@ ValueNum ValueNumStore::VNApplySelectorsTypeCheck(ValueNum elem, var_types indTy
         size_t elemTypSize = (elemTyp == TYP_STRUCT) ? elemStructSize : genTypeSize(elemTyp);
         size_t indTypeSize = genTypeSize(indType);
 
-        if ((indType == TYP_REF) && (varTypeIsStruct(elemTyp)))
-        {
-            // indType is TYP_REF and elemTyp is TYP_STRUCT
-            //
-            // We have a pointer to a static that is a Boxed Struct
-            //
-            return elem;
-        }
-        else if (indTypeSize > elemTypSize)
+        if (indTypeSize > elemTypSize)
         {
             // Reading beyong the end of 'elem'
 

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/Runtime_33884/Runtime_33884.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/Runtime_33884/Runtime_33884.cs
new file mode 100644 (file)
index 0000000..f2576c7
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/Runtime_33884/Runtime_33884.cs
@@ -0,0 +1,34 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+// The test showed CSE issues with struct return retyping.
+
+using System;
+using System.Runtime.CompilerServices;
+
+struct RefWrapper
+{
+    public Object a; // a ref field
+}
+
+class TestStructs
+{
+    static RefWrapper[] arr;
+
+    public static RefWrapper GetElement() // 8 byte size return will be retyped as a ref.
+    {
+        return arr[0];
+    }
+
+    public static int Main()
+    {
+        RefWrapper a = new RefWrapper();
+        arr = new RefWrapper[1];
+        arr[0] = a;
+
+        RefWrapper e = GetElement(); // force struct retyping to ref.
+        arr[0] = e; // a struct typed copy.
+        return 100;
+    }
+}

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/Runtime_33884/Runtime_33884.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/Runtime_33884/Runtime_33884.csproj
new file mode 100644 (file)
index 0000000..1100f42
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/Runtime_33884/Runtime_33884.csproj
@@ -0,0 +1,10 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>