Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
87 changes: 47 additions & 40 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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;
}

Expand All @@ -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()));
Expand All @@ -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);
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -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))
{
Expand All @@ -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;
}
Expand Down Expand Up @@ -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));
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
16 changes: 14 additions & 2 deletions src/coreclr/jit/layout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}
SingleAccretion marked this conversation as resolved.
Show resolved Hide resolved

CORINFO_CLASS_HANDLE clsHnd1 = layout1->GetClassHandle();
CORINFO_CLASS_HANDLE clsHnd2 = layout2->GetClassHandle();

Expand All @@ -419,6 +426,11 @@ bool ClassLayout::AreCompatible(const ClassLayout* layout1, const ClassLayout* l
return false;
}

if (layout1->GetType() != layout2->GetType())
{
return false;
}
SingleAccretion marked this conversation as resolved.
Show resolved Hide resolved

if (!layout1->HasGCPtr() && !layout2->HasGCPtr())
{
return true;
Expand Down
13 changes: 12 additions & 1 deletion src/coreclr/jit/layout.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment on lines +38 to +39
Copy link
Contributor Author

@SingleAccretion SingleAccretion May 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This increases the size of ClassLayout by one pointer. I measured the overall impact on memory consumption when CG-ing x64 CoreLib, it looked insignificant:

./diff-mem x64
Relative difference is: 0.007%

(This is a memory-CPU tradeoff: impNormStructType is not a cheap function)


// Class name as reported by ICorJitInfo::getClassName
INDEBUG(const char* m_className;)

Expand All @@ -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")
Expand All @@ -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)
Expand All @@ -73,6 +78,7 @@ class ClassLayout
#endif
, m_gcPtrCount(0)
, m_gcPtrs(nullptr)
, m_type(type)
#ifdef DEBUG
, m_className(className)
, m_shortClassName(shortClassName)
Expand Down Expand Up @@ -120,6 +126,11 @@ class ClassLayout
return m_size;
}

var_types GetType() const
{
return m_type;
}

//------------------------------------------------------------------------
// GetRegisterType: Determine register type for the layout.
//
Expand Down
31 changes: 31 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_69232/Runtime_69232.cs
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
@@ -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>
3 changes: 0 additions & 3 deletions src/tests/issues.targets
Original file line number Diff line number Diff line change
Expand Up @@ -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 -->
Expand Down