Skip to content

Commit

Permalink
Improve "one asg morphing" and move it to morphblock.cpp (#79037)
Browse files Browse the repository at this point in the history 
* Enhance OneAsg morphing

To also consider RHS types, in case the RHS wasn't suitable.

TODO: move the whole thing to the general block morphing.

* Move OneAsg morphing to morphblock.cpp

* Disable in minopts except for small types

The main point of the transformation is to keep things DNERless.

Exclude small types because for them the transformation is required
in case of "full" stores into normalize-on-store locals.
  • Loading branch information
@SingleAccretion
SingleAccretion authored Dec 15, 2022
1 parent 381c782 commit caa0bd5
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 134 deletions.
5 changes: 0 additions & 5 deletions src/coreclr/jit/compiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10094,10 +10094,6 @@ void Compiler::EnregisterStats::RecordLocal(const LclVarDsc* varDsc)
m_storeBlkSrc++;
break;

case DoNotEnregisterReason::OneAsgRetyping:
m_oneAsgRetyping++;
break;

case DoNotEnregisterReason::SwizzleArg:
m_swizzleArg++;
break;
Expand Down Expand Up @@ -10232,7 +10228,6 @@ void Compiler::EnregisterStats::Dump(FILE* fout) const
PRINT_STATS(m_lclAddrNode, notEnreg);
PRINT_STATS(m_castTakesAddr, notEnreg);
PRINT_STATS(m_storeBlkSrc, notEnreg);
PRINT_STATS(m_oneAsgRetyping, notEnreg);
PRINT_STATS(m_swizzleArg, notEnreg);
PRINT_STATS(m_blockOpRet, notEnreg);
PRINT_STATS(m_returnSpCheck, notEnreg);
Expand Down
3 changes: 0 additions & 3 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,6 @@ enum class DoNotEnregisterReason
LclAddrNode, // the local is accessed with LCL_ADDR_VAR/FLD.
CastTakesAddr,
StoreBlkSrc, // the local is used as STORE_BLK source.
OneAsgRetyping, // fgMorphOneAsgBlockOp prevents this local from being enregister.
SwizzleArg, // the local is passed using LCL_FLD as another type.
BlockOpRet, // the struct is returned and it promoted or there is a cast.
ReturnSpCheck, // the local is used to do SP check on return from function
Expand Down Expand Up @@ -5763,7 +5762,6 @@ class Compiler
CORINFO_CONTEXT_HANDLE* ExactContextHnd,
methodPointerInfo* ldftnToken);
GenTree* fgMorphLeaf(GenTree* tree);
GenTree* fgMorphOneAsgBlockOp(GenTree* tree);
GenTree* fgMorphInitBlock(GenTree* tree);
GenTree* fgMorphCopyBlock(GenTree* tree);
GenTree* fgMorphStoreDynBlock(GenTreeStoreDynBlk* tree);
Expand Down Expand Up @@ -10102,7 +10100,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
unsigned m_lclAddrNode;
unsigned m_castTakesAddr;
unsigned m_storeBlkSrc;
unsigned m_oneAsgRetyping;
unsigned m_swizzleArg;
unsigned m_blockOpRet;
unsigned m_returnSpCheck;
Expand Down
4 changes: 0 additions & 4 deletions src/coreclr/jit/lclvars.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2781,10 +2781,6 @@ void Compiler::lvaSetVarDoNotEnregister(unsigned varNum DEBUGARG(DoNotEnregister
JITDUMP("the local is used as store block src\n");
break;

case DoNotEnregisterReason::OneAsgRetyping:
JITDUMP("OneAsg forbids enreg\n");
break;

case DoNotEnregisterReason::SwizzleArg:
JITDUMP("SwizzleArg\n");
break;
Expand Down
99 changes: 0 additions & 99 deletions src/coreclr/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8495,105 +8495,6 @@ void Compiler::fgAssignSetVarDef(GenTree* tree)
}
}

//------------------------------------------------------------------------
// fgMorphOneAsgBlockOp: Attempt to replace a block assignment with a scalar assignment
//
// Arguments:
// tree - The block assignment to be possibly morphed
//
// Return Value:
// The modified tree if successful, nullptr otherwise.
//
// Assumptions:
// 'tree' must be a block assignment.
//
// Notes:
// If successful, this method always returns the incoming tree, modifying only
// its arguments.
//
GenTree* Compiler::fgMorphOneAsgBlockOp(GenTree* tree)
{
// This must be a block assignment.
assert(tree->OperIsCopyBlkOp());

if (!tree->TypeIs(TYP_STRUCT))
{
return nullptr;
}

var_types asgType = TYP_UNDEF;
GenTree* asg = tree;
GenTree* dest = asg->gtGetOp1();
GenTree* src = asg->gtGetOp2();
LclVarDsc* destVarDsc = nullptr;
assert((src == src->gtEffectiveVal()) && (dest == dest->gtEffectiveVal()));

if (dest->OperIs(GT_LCL_FLD))
{
destVarDsc = lvaGetDesc(dest->AsLclFld());
asgType = destVarDsc->TypeGet();

// We will use the dest local directly.
if (!varTypeIsIntegralOrI(asgType) || (dest->AsLclFld()->GetSize() != genTypeSize(asgType)))
{
return nullptr;
}
}
else
{
return nullptr;
}

if (!src->OperIsIndir() && !src->OperIsLocalRead())
{
// We cannot easily retype other nodes.
return nullptr;
}

if (src->OperIs(GT_LCL_FLD) && (lvaGetDesc(src->AsLclFld())->TypeGet() == asgType))
{
src->SetOper(GT_LCL_VAR);
src->ChangeType(asgType);
}
else if (src->OperIs(GT_LCL_VAR) && lvaGetDesc(src->AsLclVar())->lvPromoted)
{
// Leave handling these to block morphing.
return nullptr;
}

// If the block operation had been a write to a local var of a small int type,
// of the exact size of the small int type, and the var is NormalizeOnStore,
// we would have labeled it GTF_VAR_USEASG, because the block operation wouldn't
// have done that normalization. If we're now making it into an assignment,
// the NormalizeOnStore will work, and it can be a full def.
dest->SetOper(GT_LCL_VAR);
dest->ChangeType(destVarDsc->lvNormalizeOnLoad() ? asgType : genActualType(asgType));
dest->gtFlags &= ~GTF_VAR_USEASG;

// Retype the RHS.
assert(varTypeIsIntegralOrI(asgType));
if (src->OperIsBlk())
{
src->SetOper(GT_IND);
}
else if (src->OperIs(GT_LCL_VAR) && !src->TypeIs(asgType))
{
lvaSetVarDoNotEnregister(src->AsLclVar()->GetLclNum() DEBUGARG(DoNotEnregisterReason::OneAsgRetyping));
src->SetOper(GT_LCL_FLD);
}
src->ChangeType(asgType);

// Set the lhs and rhs on the assignment.
asg->AsOp()->gtOp1 = dest;
asg->AsOp()->gtOp2 = src;
asg->ChangeType(asgType);

JITDUMP("fgMorphOneAsgBlock (after):\n");
DISPTREE(tree);

return tree;
}

#ifdef FEATURE_SIMD

//--------------------------------------------------------------------------------------------------------------
Expand Down
116 changes: 93 additions & 23 deletions src/coreclr/jit/morphblock.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,22 +134,7 @@ GenTree* MorphInitBlockHelper::Morph()

if (m_transformationDecision == BlockTransformation::Undefined)
{
GenTree* oneAsgTree = nullptr;
if (!m_initBlock)
{
oneAsgTree = m_comp->fgMorphOneAsgBlockOp(m_asg);
}
if (oneAsgTree != nullptr)
{
assert((m_asg == oneAsgTree) && "fgMorphOneAsgBlock must return the incoming tree.");

m_transformationDecision = BlockTransformation::OneAsgBlock;
m_result = oneAsgTree;
}
else
{
MorphStructCases();
}
MorphStructCases();
}

PropagateExpansionAssertions();
Expand Down Expand Up @@ -300,7 +285,7 @@ void MorphInitBlockHelper::TrySpecialCases()
// but sets appropriate flags for the involved lclVars.
//
// Assumptions:
// we have already checked that it is not a special case and can't be transformed as OneAsgBlock.
// we have already checked that it is not a special case.
//
void MorphInitBlockHelper::MorphStructCases()
{
Expand Down Expand Up @@ -358,7 +343,7 @@ GenTree* MorphInitBlockHelper::MorphBlock(Compiler* comp, GenTree* tree, bool is
if (tree->OperIs(GT_COMMA))
{
// TODO-Cleanup: this block is not needed for not struct nodes, but
// fgMorphOneAsgBlockOp works wrong without this transformation.
// TryPrimitiveCopy works wrong without this transformation.
tree = MorphCommaBlock(comp, tree->AsOp());
if (isDest)
{
Expand Down Expand Up @@ -682,6 +667,7 @@ class MorphCopyBlockHelper : public MorphInitBlockHelper
void TrySpecialCases() override;

void MorphStructCases() override;
void TryPrimitiveCopy();
GenTree* CopyFieldByField();

const char* GetHelperName() const override
Expand Down Expand Up @@ -805,7 +791,7 @@ void MorphCopyBlockHelper::TrySpecialCases()
// but sets appropriate flags for the involved lclVars.
//
// Assumptions:
// we have already checked that it is not a special case and can't be transformed as OneAsgBlock.
// We have already checked that it is not a special case.
//
void MorphCopyBlockHelper::MorphStructCases()
{
Expand Down Expand Up @@ -1052,8 +1038,13 @@ void MorphCopyBlockHelper::MorphStructCases()

if (requiresCopyBlock)
{
m_result = m_asg;
m_transformationDecision = BlockTransformation::StructBlock;
TryPrimitiveCopy();

if (m_transformationDecision == BlockTransformation::Undefined)
{
m_result = m_asg;
m_transformationDecision = BlockTransformation::StructBlock;
}
}
else
{
Expand Down Expand Up @@ -1089,6 +1080,86 @@ void MorphCopyBlockHelper::MorphStructCases()
}
}

//------------------------------------------------------------------------
// TryPrimitiveCopy: Attempt to replace a block assignment with a scalar assignment.
//
// If successful, will set "m_transformationDecision" to "OneAsgBlock".
//
void MorphCopyBlockHelper::TryPrimitiveCopy()
{
if (!m_dst->TypeIs(TYP_STRUCT) || (m_blockSize == 0))
{
return;
}

if (m_comp->opts.OptimizationDisabled() && (m_blockSize >= genTypeSize(TYP_INT)))
{
return;
}

var_types asgType = TYP_UNDEF;
assert((m_src == m_src->gtEffectiveVal()) && (m_dst == m_dst->gtEffectiveVal()));

// Can we use the LHS local directly?
if (m_dst->OperIs(GT_LCL_FLD))
{
if (m_blockSize == genTypeSize(m_dstVarDsc))
{
asgType = m_dstVarDsc->TypeGet();
}
}
else if (!m_dst->OperIsIndir())
{
return;
}

if (m_srcVarDsc != nullptr)
{
if ((asgType == TYP_UNDEF) && (m_blockSize == genTypeSize(m_srcVarDsc)))
{
asgType = m_srcVarDsc->TypeGet();
}
}
else if (!m_src->OperIsIndir())
{
return;
}

if (asgType == TYP_UNDEF)
{
return;
}

auto doRetypeNode = [asgType](GenTree* op, LclVarDsc* varDsc) {
if (op->OperIsIndir())
{
op->SetOper(GT_IND);
op->ChangeType(asgType);
}
else if (varDsc->TypeGet() == asgType)
{
op->SetOper(GT_LCL_VAR);
op->ChangeType(varDsc->lvNormalizeOnLoad() ? varDsc->TypeGet() : genActualType(varDsc));
op->gtFlags &= ~GTF_VAR_USEASG;
}
else
{
if (op->OperIs(GT_LCL_VAR))
{
op->SetOper(GT_LCL_FLD);
}
op->ChangeType(asgType);
}
};

doRetypeNode(m_dst, m_dstVarDsc);
doRetypeNode(m_src, m_srcVarDsc);
m_asg->ChangeType(asgType);

m_result = m_asg;
m_transformationDecision = BlockTransformation::OneAsgBlock;
}

//------------------------------------------------------------------------
// CopyFieldByField: transform the copy block to a field by field assignment.
//
Expand Down Expand Up @@ -1438,7 +1509,7 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField()
//
// Return Value:
// We can return the original block copy unmodified (least desirable, but always correct)
// We can return a single assignment, when fgMorphOneAsgBlockOp transforms it (most desirable).
// We can return a single assignment, when TryPrimitiveCopy transforms it (most desirable).
// If we have performed struct promotion of the Source() or the Dest() then we will try to
// perform a field by field assignment for each of the promoted struct fields.
//
Expand All @@ -1465,7 +1536,6 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
// tree - A GT_ASG tree that performs block initialization.
//
// Return Value:
// A single assignment, when fgMorphOneAsgBlockOp transforms it.
// If the destination is a promoted struct local variable then we will try to
// perform a field by field assignment for each of the promoted struct fields.
// This is not always possible (e.g. if the struct is address exposed).
Expand Down

0 comments on commit caa0bd5

Please sign in to comment.