Do not fold mismatched OBJ(ADDR(LCL_VAR)) (#69271)

* Do not fold mismatched OBJ(ADDR(LCL_VAR)) The transforms in question could fold, e. g. "OBJ<8>(ADDR(LCL_VAR<16>))" to just the "LCL_VAR", necessitating workarounds in block morphing that had to rematerialize the block node to restore IR's validity. It is better to simply not create questionable IR in the first place. * Add a test * Re-enable tests * Fix regressions By using layout compatibility in "gtNewStructVal" instead of handle equality. This is safe to do because "gtNewStructVal" is only used in contexts of block copies (as opposed to call arguments).
dotnet · May 14, 2022 · 6134ee9 · 6134ee9
1 parent af2a48d
commit 6134ee9
Show file tree

Hide file tree

Showing 8 changed files with 117 additions and 48 deletions.
diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h
@@ -2303,9 +2303,10 @@ class Compiler
     void gtBlockOpInit(GenTree* result, GenTree* dst, GenTree* srcOrFillVal, bool isVolatile);
 
 public:
+    GenTreeObj* gtNewObjNode(ClassLayout* layout, GenTree* addr);
     GenTreeObj* gtNewObjNode(CORINFO_CLASS_HANDLE structHnd, GenTree* addr);
     void gtSetObjGcInfo(GenTreeObj* objNode);
-    GenTree* gtNewStructVal(CORINFO_CLASS_HANDLE structHnd, GenTree* addr);
+    GenTree* gtNewStructVal(ClassLayout* layout, GenTree* addr);
     GenTree* gtNewBlockVal(GenTree* addr, unsigned size);
 
     GenTree* gtNewCpObjNode(GenTree* dst, GenTree* src, CORINFO_CLASS_HANDLE structHnd, bool isVolatile);

diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp
@@ -7196,21 +7196,20 @@ GenTreeOp* Compiler::gtNewAssignNode(GenTree* dst, GenTree* src)
 }
 
 //------------------------------------------------------------------------
-// gtNewObjNode: Creates a new Obj node.
+// gtNewObjNode: Creates a new Obj node with the given layout.
 //
 // Arguments:
-//    structHnd - The class handle of the struct type.
-//    addr      - The address of the struct.
+//    layout - The struct layout
+//    addr   - The address of the struct
 //
 // Return Value:
 //    Returns a node representing the struct value at the given address.
 //
-GenTreeObj* Compiler::gtNewObjNode(CORINFO_CLASS_HANDLE structHnd, GenTree* addr)
+GenTreeObj* Compiler::gtNewObjNode(ClassLayout* layout, GenTree* addr)
 {
-    var_types nodeType = impNormStructType(structHnd);
-    assert(varTypeIsStruct(nodeType));
+    assert(layout != nullptr);
 
-    GenTreeObj* objNode = new (this, GT_OBJ) GenTreeObj(nodeType, addr, typGetObjLayout(structHnd));
+    GenTreeObj* objNode = new (this, GT_OBJ) GenTreeObj(layout->GetType(), addr, layout);
 
     // An Obj is not a global reference, if it is known to be a local struct.
     if ((addr->gtFlags & GTF_GLOB_REF) == 0)
@@ -7225,6 +7224,25 @@ GenTreeObj* Compiler::gtNewObjNode(CORINFO_CLASS_HANDLE structHnd, GenTree* addr
             }
         }
     }
+
+    return objNode;
+}
+
+//------------------------------------------------------------------------
+// gtNewObjNode: Creates a new Obj node with the layout for the given handle.
+//
+// Arguments:
+//    structHnd - The class handle of the struct type
+//    addr      - The address of the struct
+//
+// Return Value:
+//    Returns a node representing the struct value at the given address.
+//
+GenTreeObj* Compiler::gtNewObjNode(CORINFO_CLASS_HANDLE structHnd, GenTree* addr)
+{
+    ClassLayout* layout  = typGetObjLayout(structHnd);
+    GenTreeObj*  objNode = gtNewObjNode(layout, addr);
+
     return objNode;
 }
 
@@ -7233,7 +7251,7 @@ GenTreeObj* Compiler::gtNewObjNode(CORINFO_CLASS_HANDLE structHnd, GenTree* addr
 //
 // Arguments:
 //    objNode - The object node of interest
-
+//
 void Compiler::gtSetObjGcInfo(GenTreeObj* objNode)
 {
     assert(varTypeIsStruct(objNode->TypeGet()));
@@ -7249,28 +7267,31 @@ void Compiler::gtSetObjGcInfo(GenTreeObj* objNode)
 // gtNewStructVal: Return a node that represents a struct value
 //
 // Arguments:
-//    structHnd - The class for the struct
-//    addr      - The address of the struct
+//    layout - The struct's layout
+//    addr   - The address of the struct
 //
 // Return Value:
-//    A block, object or local node that represents the struct value pointed to by 'addr'.
-
-GenTree* Compiler::gtNewStructVal(CORINFO_CLASS_HANDLE structHnd, GenTree* addr)
+//    An "OBJ" node, or "LCL_VAR" node if "addr" points to a local with
+//    a layout compatible with "layout".
+//
+GenTree* Compiler::gtNewStructVal(ClassLayout* layout, GenTree* addr)
 {
-    if (addr->gtOper == GT_ADDR)
+    if (addr->OperIs(GT_ADDR))
     {
-        GenTree* val = addr->gtGetOp1();
-        if (val->OperGet() == GT_LCL_VAR)
+        GenTree* location = addr->gtGetOp1();
+        if (location->OperIs(GT_LCL_VAR))
         {
-            unsigned   lclNum = addr->gtGetOp1()->AsLclVarCommon()->GetLclNum();
-            LclVarDsc* varDsc = &(lvaTable[lclNum]);
-            if (varTypeIsStruct(varDsc) && (varDsc->GetStructHnd() == structHnd) && !lvaIsImplicitByRefLocal(lclNum))
+            unsigned   lclNum = location->AsLclVar()->GetLclNum();
+            LclVarDsc* varDsc = lvaGetDesc(lclNum);
+            if (!lvaIsImplicitByRefLocal(lclNum) && varTypeIsStruct(varDsc) &&
+                ClassLayout::AreCompatible(layout, varDsc->GetLayout()))
             {
-                return addr->gtGetOp1();
+                return location;
             }
         }
     }
-    return gtNewObjNode(structHnd, addr);
+
+    return gtNewObjNode(layout, addr);
 }
 
 //------------------------------------------------------------------------
@@ -7320,8 +7341,9 @@ GenTree* Compiler::gtNewBlockVal(GenTree* addr, unsigned size)
 
 GenTree* Compiler::gtNewCpObjNode(GenTree* dstAddr, GenTree* srcAddr, CORINFO_CLASS_HANDLE structHnd, bool isVolatile)
 {
-    GenTree* lhs = gtNewStructVal(structHnd, dstAddr);
-    GenTree* src = nullptr;
+    ClassLayout* layout = typGetObjLayout(structHnd);
+    GenTree*     lhs    = gtNewStructVal(layout, dstAddr);
+    GenTree*     src    = gtNewStructVal(layout, srcAddr);
 
     if (lhs->OperIs(GT_OBJ))
     {
@@ -7340,15 +7362,6 @@ GenTree* Compiler::gtNewCpObjNode(GenTree* dstAddr, GenTree* srcAddr, CORINFO_CL
         gtSetObjGcInfo(lhsObj);
     }
 
-    if (srcAddr->OperGet() == GT_ADDR)
-    {
-        src = srcAddr->AsOp()->gtOp1;
-    }
-    else
-    {
-        src = gtNewOperNode(GT_IND, lhs->TypeGet(), srcAddr);
-    }
-
     GenTree* result = gtNewBlkOpNode(lhs, src, isVolatile, true);
     return result;
 }
@@ -7693,14 +7706,8 @@ void Compiler::gtBlockOpInit(GenTree* result, GenTree* dst, GenTree* srcOrFillVa
 GenTree* Compiler::gtNewBlkOpNode(GenTree* dst, GenTree* srcOrFillVal, bool isVolatile, bool isCopyBlock)
 {
     assert(dst->OperIsBlk() || dst->OperIsLocal());
-    if (isCopyBlock)
-    {
-        if (srcOrFillVal->OperIsIndir() && (srcOrFillVal->gtGetOp1()->gtOper == GT_ADDR))
-        {
-            srcOrFillVal = srcOrFillVal->gtGetOp1()->gtGetOp1();
-        }
-    }
-    else
+
+    if (!isCopyBlock)
     {
         // InitBlk
         assert(varTypeIsIntegral(srcOrFillVal));

diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp
@@ -17203,7 +17203,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
 
                 if (eeIsValueClass(resolvedToken.hClass))
                 {
-                    op1 = gtNewStructVal(resolvedToken.hClass, op1);
+                    op1 = gtNewStructVal(typGetObjLayout(resolvedToken.hClass), op1);
                     if (op1->OperIs(GT_OBJ))
                     {
                         gtSetObjGcInfo(op1->AsObj());

diff --git a/src/coreclr/jit/layout.cpp b/src/coreclr/jit/layout.cpp
@@ -334,12 +334,15 @@ ClassLayout* ClassLayout::Create(Compiler* compiler, CORINFO_CLASS_HANDLE classH
         size = compiler->info.compCompHnd->getHeapClassSize(classHandle);
     }
 
+    var_types type = compiler->impNormStructType(classHandle);
+
     INDEBUG(const char* className = compiler->eeGetClassName(classHandle);)
     INDEBUG(const char16_t* shortClassName = compiler->eeGetShortClassName(classHandle);)
 
     ClassLayout* layout = new (compiler, CMK_ClassLayout)
-        ClassLayout(classHandle, isValueClass, size DEBUGARG(className) DEBUGARG(shortClassName));
+        ClassLayout(classHandle, isValueClass, size, type DEBUGARG(className) DEBUGARG(shortClassName));
     layout->InitializeGCPtrs(compiler);
+
     return layout;
 }
 
@@ -400,7 +403,11 @@ void ClassLayout::InitializeGCPtrs(Compiler* compiler)
 // static
 bool ClassLayout::AreCompatible(const ClassLayout* layout1, const ClassLayout* layout2)
 {
-    assert((layout1 != nullptr) && (layout2 != nullptr));
+    if ((layout1 == nullptr) || (layout2 == nullptr))
+    {
+        return false;
+    }
+
     CORINFO_CLASS_HANDLE clsHnd1 = layout1->GetClassHandle();
     CORINFO_CLASS_HANDLE clsHnd2 = layout2->GetClassHandle();
 
@@ -419,6 +426,11 @@ bool ClassLayout::AreCompatible(const ClassLayout* layout1, const ClassLayout* l
         return false;
     }
 
+    if (layout1->GetType() != layout2->GetType())
+    {
+        return false;
+    }
+
     if (!layout1->HasGCPtr() && !layout2->HasGCPtr())
     {
         return true;

diff --git a/src/coreclr/jit/layout.h b/src/coreclr/jit/layout.h
@@ -35,6 +35,9 @@ class ClassLayout
         BYTE  m_gcPtrsArray[sizeof(BYTE*)];
     };
 
+    // The normalized type to use in IR for block nodes with this layout.
+    const var_types m_type;
+
     // Class name as reported by ICorJitInfo::getClassName
     INDEBUG(const char* m_className;)
 
@@ -53,6 +56,7 @@ class ClassLayout
 #endif
         , m_gcPtrCount(0)
         , m_gcPtrs(nullptr)
+        , m_type(TYP_STRUCT)
 #ifdef DEBUG
         , m_className("block")
         , m_shortClassName(u"block")
@@ -64,7 +68,8 @@ class ClassLayout
 
     ClassLayout(CORINFO_CLASS_HANDLE classHandle,
                 bool                 isValueClass,
-                unsigned size DEBUGARG(const char* className) DEBUGARG(const char16_t* shortClassName))
+                unsigned             size,
+                var_types type DEBUGARG(const char* className) DEBUGARG(const char16_t* shortClassName))
         : m_classHandle(classHandle)
         , m_size(size)
         , m_isValueClass(isValueClass)
@@ -73,6 +78,7 @@ class ClassLayout
 #endif
         , m_gcPtrCount(0)
         , m_gcPtrs(nullptr)
+        , m_type(type)
 #ifdef DEBUG
         , m_className(className)
         , m_shortClassName(shortClassName)
@@ -120,6 +126,11 @@ class ClassLayout
         return m_size;
     }
 
+    var_types GetType() const
+    {
+        return m_type;
+    }
+
     //------------------------------------------------------------------------
     // GetRegisterType: Determine register type for the layout.
     //

diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_69232/Runtime_69232.cs b/src/tests/JIT/Regression/JitBlue/Runtime_69232/Runtime_69232.cs
@@ -0,0 +1,31 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Numerics;
+using System.Runtime.CompilerServices;
+
+public unsafe class Runtime_69232
+{
+    public static int Main()
+    {
+        return Problem(new(1, 1, 1, 1)) ? 101 : 100;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static bool Problem(Vector4 vtor)
+    {
+        FakeVtor a = GetFakeVtor(vtor);
+
+        return a.FirstFlt != 1;
+    }
+
+    private static FakeVtor GetFakeVtor(Vector4 v) => *(FakeVtor*)&v;
+
+    struct FakeVtor
+    {
+        public float FirstFlt;
+        public float SecondFlt;
+        public float ThirdFlt;
+        public float FourthFlt;
+    }
+}
diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_69232/Runtime_69232.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_69232/Runtime_69232.csproj
@@ -0,0 +1,10 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <Optimize>True</Optimize>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>
diff --git a/src/tests/issues.targets b/src/tests/issues.targets
@@ -250,9 +250,6 @@
         <ExcludeList Include = "$(XunitTestBinBase)/JIT/HardwareIntrinsics/X86/**">
             <Issue>https://github.com/dotnet/runtime/issues/60154</Issue>
         </ExcludeList>
-        <ExcludeList Include="$(XunitTestBinBase)/JIT/HardwareIntrinsics/General/NotSupported/**">
-            <Issue>https://github.com/dotnet/runtime/issues/69232</Issue>
-        </ExcludeList>
     </ItemGroup>
 
     <!-- Windows all architecture excludes -->