From baec011a7b853a6cca46f47f651e68a01b5029e5 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 12 May 2022 21:11:16 +0300 Subject: [PATCH 1/4] 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. --- src/coreclr/jit/gentree.cpp | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index d9234846e2269b..2e1c33600a0551 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -7321,7 +7321,7 @@ 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; + GenTree* src = gtNewStructVal(structHnd, srcAddr); if (lhs->OperIs(GT_OBJ)) { @@ -7340,15 +7340,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 +7684,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)); From 483737345bca13dc0ee317f9da0dfca96bac48d7 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 12 May 2022 21:59:49 +0300 Subject: [PATCH 2/4] Add a test --- .../JitBlue/Runtime_69232/Runtime_69232.cs | 31 +++++++++++++++++++ .../Runtime_69232/Runtime_69232.csproj | 10 ++++++ 2 files changed, 41 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_69232/Runtime_69232.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_69232/Runtime_69232.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_69232/Runtime_69232.cs b/src/tests/JIT/Regression/JitBlue/Runtime_69232/Runtime_69232.cs new file mode 100644 index 00000000000000..53e0d546e23c49 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..cf94135633b19a --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_69232/Runtime_69232.csproj @@ -0,0 +1,10 @@ + + + Exe + True + true + + + + + \ No newline at end of file From 37a967a7995ac25403f2f65285e4eba353b98b0f Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Thu, 12 May 2022 22:01:57 +0300 Subject: [PATCH 3/4] Re-enable tests --- src/tests/issues.targets | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/tests/issues.targets b/src/tests/issues.targets index 265461e41ab0e6..a12139bd64117c 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -250,9 +250,6 @@ https://github.com/dotnet/runtime/issues/60154 - - https://github.com/dotnet/runtime/issues/69232 - From 565ef7f2b94db9f52b9901832b1536c44a1f2c79 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sat, 14 May 2022 00:13:15 +0300 Subject: [PATCH 4/4] 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). --- src/coreclr/jit/compiler.h | 3 +- src/coreclr/jit/gentree.cpp | 68 ++++++++++++++++++++++++------------ src/coreclr/jit/importer.cpp | 2 +- src/coreclr/jit/layout.cpp | 16 +++++++-- src/coreclr/jit/layout.h | 13 ++++++- 5 files changed, 74 insertions(+), 28 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index d7ebb305c99124..e16467b3ed4a37 100644 --- 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 index 2e1c33600a0551..d00129b1972a46 100644 --- 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 = gtNewStructVal(structHnd, srcAddr); + ClassLayout* layout = typGetObjLayout(structHnd); + GenTree* lhs = gtNewStructVal(layout, dstAddr); + GenTree* src = gtNewStructVal(layout, srcAddr); if (lhs->OperIs(GT_OBJ)) { diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 3b0a7795c5b9a7..e2360e7f7c80d6 100644 --- 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 index 1e71517c897dae..d420f9f57b1d08 100644 --- 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 index b2d37c4163480f..01040f8eede39b 100644 --- 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. //