Skip to content

Commit

Permalink
Fold primitive-typed access to promoted structs in local morph and fo…
Browse files Browse the repository at this point in the history 
…rbid mismatched struct assignments (#76766)

* GenTree::GetLayout field FIELDs

* Do not create invalid IR when replacing promoted fields

Instead, create constructions like "OBJ(ADDR(LCL_VAR))"
that block morphing can recognize on its own.

* Delete lvFieldHnd

* GenTree::GetLayout - CALL/COMMA

* GenTreeHWIntrinsic::GetLayout

* Add an assert that LHS and RHS match

* Fix assert violation

And simplify the code...

* Delete RetypedAsScalarFieldsMap

Without handle equality, the assert no longer holds, for example:
```cs
private StructWithInt Problem(StructWithInt b, StructWithInt a)
{
    a = ((StructWithStructWithInt*)&b)->StructWithInt;
    return a;
}
```

* Delete a bit of code

Dead / unncessary.

No diffs.

* Fold promoted locals in local morph

* Support GT_IND in MorphStructField

This is significantly simpler than moving the promotion logic
to post-order because we don't need to fiddle with the ref
counting process and complexities of intermediate states, e. g.
"IND<float>(ADDR(FIELD<int>(ADDR(LCL_VAR<struct>))))" can be
naturally turned into "BITCAST<float>(LCL_VAR<int>)" like this.

* More code removal

* Move "fgMorphStructField" to local morph

* TP tuning

With this, we have only a very small regression:

Base: 1297463478, Diff: 1297750893, +0.0222%
LocalAddressVisitor::MorphStructField : 345099  : NA       : 31.52% : +0.0266%
LocalAddressVisitor::PreOrderVisit    : 224684  : +3.53%   : 20.52% : +0.0173%
memset                                : 88591   : +0.99%   : 8.09%  : +0.0068%
Compiler::fgMorphSmpOp                : -19170  : -0.06%   : 1.75%  : -0.0015%
Compiler::fgMorphStructField          : -367474 : -100.00% : 33.56% : -0.0283%

(This is with a dummy field on LclVarDsc)

Losing the assert does not seem worrysome as we have an identical one in "fgMorphField".
  • Loading branch information
@SingleAccretion
SingleAccretion authored Oct 24, 2022
1 parent 41db775 commit 76cf397
Show file tree
Hide file tree
Showing 8 changed files with 168 additions and 312 deletions.
13 changes: 0 additions & 13 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1021,8 +1021,6 @@ class LclVarDsc
return structHnd;
}

CORINFO_FIELD_HANDLE lvFieldHnd; // field handle for promoted struct fields

private:
ClassLayout* m_layout; // layout info for structs

Expand Down Expand Up @@ -3374,10 +3372,6 @@ class Compiler
structPromotionInfo.typeHnd = NO_CLASS_HANDLE;
}

#ifdef DEBUG
void CheckRetypedAsScalar(CORINFO_FIELD_HANDLE fieldHnd, var_types requestedType);
#endif // DEBUG

private:
bool CanPromoteStructVar(unsigned lclNum);
bool ShouldPromoteStructVar(unsigned lclNum);
Expand All @@ -3392,12 +3386,6 @@ class Compiler
private:
Compiler* compiler;
lvaStructPromotionInfo structPromotionInfo;

#ifdef DEBUG
typedef JitHashTable<CORINFO_FIELD_HANDLE, JitPtrKeyFuncs<CORINFO_FIELD_STRUCT_>, var_types>
RetypedAsScalarFieldsMap;
RetypedAsScalarFieldsMap retypedFieldsMap;
#endif // DEBUG
};

StructPromotionHelper* structPromotionHelper;
Expand Down Expand Up @@ -5866,7 +5854,6 @@ class Compiler
#endif

PhaseStatus fgPromoteStructs();
void fgMorphStructField(GenTree* tree, GenTree* parent);
void fgMorphLocalField(GenTree* tree, GenTree* parent);

// Reset the refCount for implicit byrefs.
Expand Down
59 changes: 58 additions & 1 deletion src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,7 @@ ClassLayout* GenTree::GetLayout(Compiler* compiler) const
{
assert(varTypeIsStruct(TypeGet()));

CORINFO_CLASS_HANDLE structHnd = NO_CLASS_HANDLE;
switch (OperGet())
{
case GT_LCL_VAR:
Expand All @@ -604,12 +605,31 @@ ClassLayout* GenTree::GetLayout(Compiler* compiler) const
case GT_BLK:
return AsBlk()->GetLayout();

case GT_COMMA:
return AsOp()->gtOp2->GetLayout(compiler);

#ifdef FEATURE_HW_INTRINSICS
case GT_HWINTRINSIC:
return AsHWIntrinsic()->GetLayout(compiler);
#endif // FEATURE_HW_INTRINSICS

case GT_MKREFANY:
return compiler->typGetObjLayout(compiler->impGetRefAnyClass());
structHnd = compiler->impGetRefAnyClass();
break;

case GT_FIELD:
compiler->eeGetFieldType(AsField()->gtFldHnd, &structHnd);
break;

case GT_CALL:
structHnd = AsCall()->gtRetClsHnd;
break;

default:
unreached();
}

return compiler->typGetObjLayout(structHnd);
}

//-----------------------------------------------------------
Expand Down Expand Up @@ -22853,6 +22873,43 @@ bool GenTreeHWIntrinsic::OperIsMemoryLoadOrStore() const
return OperIsMemoryLoad() || OperIsMemoryStore();
}

//------------------------------------------------------------------------
// GetLayout: Get the layout for this TYP_STRUCT HWI node.
//
// Arguments:
// compiler - The Compiler instance
//
// Return Value:
// The struct layout for the node.
//
// Notes:
// Currently, this method synthesizes block layouts, since there is not
// enough space in the GenTreeHWIntrinsic node to store a proper layout
// pointer or number.
//
ClassLayout* GenTreeHWIntrinsic::GetLayout(Compiler* compiler) const
{
assert(TypeIs(TYP_STRUCT));

switch (GetHWIntrinsicId())
{
#ifdef TARGET_ARM64
case NI_AdvSimd_Arm64_LoadPairScalarVector64:
case NI_AdvSimd_Arm64_LoadPairScalarVector64NonTemporal:
case NI_AdvSimd_Arm64_LoadPairVector64:
case NI_AdvSimd_Arm64_LoadPairVector64NonTemporal:
return compiler->typGetBlkLayout(16);

case NI_AdvSimd_Arm64_LoadPairVector128:
case NI_AdvSimd_Arm64_LoadPairVector128NonTemporal:
return compiler->typGetBlkLayout(32);
#endif // TARGET_ARM64

default:
unreached();
}
}

NamedIntrinsic GenTreeHWIntrinsic::GetHWIntrinsicId() const
{
NamedIntrinsic id = gtHWIntrinsicId;
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/jit/gentree.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6363,6 +6363,8 @@ struct GenTreeHWIntrinsic : public GenTreeJitIntrinsic

unsigned GetResultOpNumForFMA(GenTree* use, GenTree* op1, GenTree* op2, GenTree* op3);

ClassLayout* GetLayout(Compiler* compiler) const;

NamedIntrinsic GetHWIntrinsicId() const;

//---------------------------------------------------------------------------------------
Expand Down
115 changes: 94 additions & 21 deletions src/coreclr/jit/lclmorph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
{
GenTree* const node = *use;

if (node->OperIs(GT_FIELD))
if (node->OperIs(GT_IND, GT_FIELD))
{
MorphStructField(node, user);
}
Expand Down Expand Up @@ -933,8 +933,6 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
indir->AsLclFld()->SetLayout(indirLayout);
lclNode = indir->AsLclVarCommon();

// Promoted locals aren't currently handled here so partial access can't be
// later be transformed into a LCL_VAR and the variable cannot be enregistered.
m_compiler->lvaSetVarDoNotEnregister(lclNum DEBUGARG(DoNotEnregisterReason::LocalField));
break;

Expand Down Expand Up @@ -1000,13 +998,6 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>

if (indir->TypeGet() != TYP_STRUCT)
{
if (varDsc->lvPromoted)
{
// TODO-ADDR: support promoted locals here by moving the promotion morphing
// from pre-order to post-order.
return IndirTransform::None;
}

if (indir->TypeGet() == varDsc->TypeGet())
{
return IndirTransform::LclVar;
Expand Down Expand Up @@ -1082,23 +1073,105 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
}

//------------------------------------------------------------------------
// MorphStructField: Replaces a GT_FIELD based promoted/normed struct field access
// (e.g. FIELD(ADDR(LCL_VAR))) with a GT_LCL_VAR that references the struct field.
// MorphStructField: Reduces indirect access to a promoted local (e.g.
// FIELD(ADDR(LCL_VAR))) to a GT_LCL_VAR that references the struct field.
//
// Arguments:
// node - the GT_FIELD node
// user - the node that uses the field
// indir - the GT_IND/GT_FIELD node
// user - the node that uses the field
//
// Notes:
// This does not do anything if the field access does not denote
// a promoted/normed struct field.
// This does not do anything if the access does not denote a promoted
// struct field.
//
void MorphStructField(GenTree* node, GenTree* user)
void MorphStructField(GenTree* indir, GenTree* user)
{
assert(node->OperIs(GT_FIELD));
// TODO-Cleanup: Move fgMorphStructField implementation here, it's not used anywhere else.
m_compiler->fgMorphStructField(node, user);
m_stmtModified |= node->OperIs(GT_LCL_VAR);
assert(indir->OperIs(GT_IND, GT_FIELD));

GenTree* objRef = indir->AsUnOp()->gtOp1;
GenTree* obj = ((objRef != nullptr) && objRef->OperIs(GT_ADDR)) ? objRef->AsOp()->gtOp1 : nullptr;

// TODO-Bug: this code does not pay attention to "GTF_IND_VOLATILE".
if ((obj != nullptr) && obj->OperIs(GT_LCL_VAR) && varTypeIsStruct(obj))
{
const LclVarDsc* varDsc = m_compiler->lvaGetDesc(obj->AsLclVarCommon());

if (varDsc->lvPromoted)
{
unsigned fieldOffset = indir->OperIs(GT_FIELD) ? indir->AsField()->gtFldOffset : 0;
unsigned fieldLclNum = m_compiler->lvaGetFieldLocal(varDsc, fieldOffset);

if (fieldLclNum == BAD_VAR_NUM)
{
// Access a promoted struct's field with an offset that doesn't correspond to any field.
// It can happen if the struct was cast to another struct with different offsets.
return;
}

const LclVarDsc* fieldDsc = m_compiler->lvaGetDesc(fieldLclNum);
var_types fieldType = fieldDsc->TypeGet();
GenTree* lclVarNode = nullptr;
GenTreeFlags lclVarFlags = indir->gtFlags & (GTF_NODE_MASK | GTF_DONT_CSE);

assert(fieldType != TYP_STRUCT); // promoted LCL_VAR can't have a struct type.
if ((indir->TypeGet() == fieldType) || ((user != nullptr) && user->OperIs(GT_ADDR)))
{
lclVarNode = indir;

if (user != nullptr)
{
if (user->OperIs(GT_ASG) && (user->AsOp()->gtOp1 == indir))
{
lclVarFlags |= GTF_VAR_DEF;
}
else if (user->OperIs(GT_ADDR))
{
// TODO-ADDR: delete this quirk.
lclVarFlags &= ~GTF_DONT_CSE;
}
}
}
else // Here we will turn "FIELD/IND(ADDR(LCL_VAR<parent>))" into "OBJ/IND(ADDR(LCL_VAR<field>))".
{
// This type mismatch is somewhat common due to how we retype fields of struct type that
// recursively simplify down to a primitive. E. g. for "struct { struct { int a } A, B }",
// the promoted local would look like "{ int a, B }", while the IR would contain "FIELD"
// nodes for the outer struct "A".
//
if (indir->TypeIs(TYP_STRUCT))
{
// TODO-1stClassStructs: delete this once "IND<struct>" nodes are no more.
if (indir->OperIs(GT_IND))
{
// We do not have a layout for this node.
return;
}

ClassLayout* layout = indir->GetLayout(m_compiler);
indir->SetOper(GT_OBJ);
indir->AsBlk()->SetLayout(layout);
indir->AsBlk()->gtBlkOpKind = GenTreeBlk::BlkOpKindInvalid;
#ifndef JIT32_GCENCODER
indir->AsBlk()->gtBlkOpGcUnsafe = false;
#endif // !JIT32_GCENCODER
}
else
{
indir->SetOper(GT_IND);
}

lclVarNode = obj;
}

lclVarNode->SetOper(GT_LCL_VAR);
lclVarNode->AsLclVarCommon()->SetLclNum(fieldLclNum);
lclVarNode->gtType = fieldType;
lclVarNode->gtFlags = lclVarFlags;

JITDUMP("Replacing the field in promoted struct with local var V%02u\n", fieldLclNum);
m_stmtModified = true;
}
}
}

//------------------------------------------------------------------------
Expand Down
36 changes: 2 additions & 34 deletions src/coreclr/jit/lclvars.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1668,12 +1668,7 @@ bool Compiler::lvaFieldOffsetCmp::operator()(const lvaStructFieldInfo& field1, c
// Arguments:
// compiler - pointer to a compiler to get access to an allocator, compHandle etc.
//
Compiler::StructPromotionHelper::StructPromotionHelper(Compiler* compiler)
: compiler(compiler)
, structPromotionInfo()
#ifdef DEBUG
, retypedFieldsMap(compiler->getAllocator(CMK_DebugOnly))
#endif // DEBUG
Compiler::StructPromotionHelper::StructPromotionHelper(Compiler* compiler) : compiler(compiler), structPromotionInfo()
{
}

Expand Down Expand Up @@ -1706,27 +1701,6 @@ bool Compiler::StructPromotionHelper::TryPromoteStructVar(unsigned lclNum)
return false;
}

#ifdef DEBUG
//--------------------------------------------------------------------------------------------
// CheckRetypedAsScalar - check that the fldType for this fieldHnd was retyped as requested type.
//
// Arguments:
// fieldHnd - the field handle;
// requestedType - as which type the field was accessed;
//
// Notes:
// For example it can happen when such struct A { struct B { long c } } is compiled and we access A.B.c,
// it could look like "GT_FIELD struct B.c -> ADDR -> GT_FIELD struct A.B -> ADDR -> LCL_VAR A" , but
// "GT_FIELD struct A.B -> ADDR -> LCL_VAR A" can be promoted to "LCL_VAR long A.B" and then
// there is type mistmatch between "GT_FIELD struct B.c" and "LCL_VAR long A.B".
//
void Compiler::StructPromotionHelper::CheckRetypedAsScalar(CORINFO_FIELD_HANDLE fieldHnd, var_types requestedType)
{
assert(retypedFieldsMap.Lookup(fieldHnd));
assert(retypedFieldsMap[fieldHnd] == requestedType);
}
#endif // DEBUG

//--------------------------------------------------------------------------------------------
// CanPromoteStructType - checks if the struct type can be promoted.
//
Expand Down Expand Up @@ -2285,9 +2259,6 @@ Compiler::lvaStructFieldInfo Compiler::StructPromotionHelper::GetFieldInfo(CORIN
{
fieldInfo.fldType = compiler->getSIMDTypeForSize(simdSize);
fieldInfo.fldSize = simdSize;
#ifdef DEBUG
retypedFieldsMap.Set(fieldInfo.fldHnd, fieldInfo.fldType, RetypedAsScalarFieldsMap::Overwrite);
#endif // DEBUG
}
}
}
Expand Down Expand Up @@ -2388,9 +2359,7 @@ bool Compiler::StructPromotionHelper::TryPromoteStructField(lvaStructFieldInfo&
// (tracked by #10019).
fieldInfo.fldType = fieldVarType;
fieldInfo.fldSize = fieldSize;
#ifdef DEBUG
retypedFieldsMap.Set(fieldInfo.fldHnd, fieldInfo.fldType, RetypedAsScalarFieldsMap::Overwrite);
#endif // DEBUG

return true;
}

Expand Down Expand Up @@ -2472,7 +2441,6 @@ void Compiler::StructPromotionHelper::PromoteStructVar(unsigned lclNum)
fieldVarDsc->lvType = pFieldInfo->fldType;
fieldVarDsc->lvExactSize = pFieldInfo->fldSize;
fieldVarDsc->lvIsStructField = true;
fieldVarDsc->lvFieldHnd = pFieldInfo->fldHnd;
fieldVarDsc->lvFldOffset = pFieldInfo->fldOffset;
fieldVarDsc->lvFldOrdinal = pFieldInfo->fldOrdinal;
fieldVarDsc->lvParentLcl = lclNum;
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/lower.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3414,8 +3414,7 @@ void Lowering::LowerRet(GenTreeUnOp* ret)
bool constStructInit = retVal->IsConstInitVal();
bool implicitCastFromSameOrBiggerSize = (genTypeSize(retActualType) <= genTypeSize(retValActualType));

// This could happen if we have retyped op1 as a primitive type during struct promotion,
// check `retypedFieldsMap` for details.
// This could happen if we have retyped op1 as a primitive type during struct promotion.
bool actualTypesMatch = (retActualType == retValActualType);

assert(actualTypesMatch || constStructInit || implicitCastFromSameOrBiggerSize);
Expand Down
Loading

0 comments on commit 76cf397

Please sign in to comment.