Skip to content

Commit

Permalink
Do not maintain GTF_VAR_MULTIREG before lowering (#75449)
Browse files Browse the repository at this point in the history
There is no need for this.
  • Loading branch information
SingleAccretion committed Sep 23, 2022
1 parent 66295dd commit 0acd508
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 106 deletions.
5 changes: 0 additions & 5 deletions src/coreclr/jit/fginline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -340,11 +340,6 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi
asg->gtOp2 = AssignStructInlineeToVar(inlinee, retClsHnd);
}
}
else if (dst->IsMultiRegLclVar())
{
// This is no longer a multi-reg assignment -- clear the flag.
dst->AsLclVar()->ClearMultiReg();
}
}

//------------------------------------------------------------------------
Expand Down
28 changes: 5 additions & 23 deletions src/coreclr/jit/forwardsub.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -469,14 +469,6 @@ bool Compiler::fgForwardSubStatement(Statement* stmt)
return false;
}

// If lhs is mulit-reg, rhs must be too.
//
if (lhsNode->IsMultiRegNode() && !fwdSubNode->IsMultiRegNode())
{
JITDUMP(" would change multi-reg (assignment)\n");
return false;
}

// Don't fwd sub overly large trees.
// Size limit here is ad-hoc. Need to tune.
//
Expand Down Expand Up @@ -663,24 +655,24 @@ bool Compiler::fgForwardSubStatement(Statement* stmt)
// There are implicit assumptions downstream on where/how multi-reg ops
// can appear.
//
// Eg if fwdSubNode is a multi-reg call, parent node must be GT_ASG and the
// local being defined must be specially marked up.
// Eg if fwdSubNode is a multi-reg call, parent node must be GT_ASG and
// the local being defined must be specially marked up.
//
if (varTypeIsStruct(fwdSubNode) && fwdSubNode->IsMultiRegCall())
if (varTypeIsStruct(fwdSubNode) && fwdSubNode->IsMultiRegNode())
{
GenTree* const parentNode = fsv.GetParentNode();

if (!parentNode->OperIs(GT_ASG))
{
JITDUMP(" multi-reg struct call, parent not asg\n");
JITDUMP(" multi-reg struct node, parent not asg\n");
return false;
}

GenTree* const parentNodeLHS = parentNode->gtGetOp1();

if (!parentNodeLHS->OperIs(GT_LCL_VAR))
{
JITDUMP(" multi-reg struct call, parent not asg(lcl, ...)\n");
JITDUMP(" multi-reg struct node, parent not asg(lcl, ...)\n");
return false;
}

Expand All @@ -691,7 +683,6 @@ bool Compiler::fgForwardSubStatement(Statement* stmt)

JITDUMP(" [marking V%02u as multi-reg-ret]", lhsLclNum);
lhsVarDsc->lvIsMultiRegRet = true;
parentNodeLHSLocal->SetMultiReg();
}

// If a method returns a multi-reg type, only forward sub locals,
Expand Down Expand Up @@ -731,18 +722,9 @@ bool Compiler::fgForwardSubStatement(Statement* stmt)

JITDUMP(" [marking V%02u as multi-reg-ret]", fwdLclNum);
fwdVarDsc->lvIsMultiRegRet = true;
fwdSubNodeLocal->SetMultiReg();
fwdSubNodeLocal->gtFlags |= GTF_DONT_CSE;
}

// If the use is a multi-reg arg, don't forward sub non-locals.
//
if (fsv.GetNode()->IsMultiRegNode() && !fwdSubNode->IsMultiRegNode())
{
JITDUMP(" would change multi-reg (substitution)\n");
return false;
}

// If the initial has truncate on store semantics, we need to replicate
// that here with a cast.
//
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,8 @@ enum GenTreeFlags : unsigned int

GTF_VAR_MULTIREG = 0x02000000, // This is a struct or (on 32-bit platforms) long variable that is used or defined
// to/from a multireg source or destination (e.g. a call arg or return, or an op
// that returns its result in multiple registers such as a long multiply).
// that returns its result in multiple registers such as a long multiply). Set by
// (and thus only valid after) lowering.

GTF_LIVENESS_MASK = GTF_VAR_DEF | GTF_VAR_USEASG | GTF_VAR_DEATH_MASK,

Expand Down
21 changes: 2 additions & 19 deletions src/coreclr/jit/hwintrinsicarm64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1800,27 +1800,10 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
}
}

GenTree* loadIntrinsic = gtNewSimdHWIntrinsicNode(retType, op1, intrinsic, simdBaseJitType, simdSize);
// This operation contains an implicit indirection
// it could point into the global heap or
// it could throw a null reference exception.
//
loadIntrinsic->gtFlags |= (GTF_GLOB_REF | GTF_EXCEPT);

assert(HWIntrinsicInfo::IsMultiReg(intrinsic));

const unsigned lclNum = lvaGrabTemp(true DEBUGARG("Return value temp for multireg intrinsic"));
impAssignTempGen(lclNum, loadIntrinsic, sig->retTypeSigClass, CHECK_SPILL_ALL);

LclVarDsc* varDsc = lvaGetDesc(lclNum);
// The following is to exclude the fields of the local to have SSA.
varDsc->lvIsMultiRegRet = true;

GenTreeLclVar* lclVar = gtNewLclvNode(lclNum, varDsc->lvType);
lclVar->SetDoNotCSE();
lclVar->SetMultiReg();

retNode = lclVar;
op1 = gtNewSimdHWIntrinsicNode(retType, op1, intrinsic, simdBaseJitType, simdSize);
retNode = impAssignMultiRegTypeToVar(op1, sig->retTypeSigClass DEBUGARG(CorInfoCallConvExtension::Managed));
break;
}

Expand Down
15 changes: 3 additions & 12 deletions src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1493,18 +1493,9 @@ GenTree* Compiler::impAssignStructPtr(GenTree* destAddr,
}
}

if (dest->OperIs(GT_LCL_VAR) &&
(src->IsMultiRegNode() ||
(src->OperIs(GT_RET_EXPR) && src->AsRetExpr()->gtInlineCandidate->AsCall()->HasMultiRegRetVal())))
if (dest->OperIs(GT_LCL_VAR) && src->IsMultiRegNode())
{
if (lvaEnregMultiRegVars && varTypeIsStruct(dest))
{
dest->AsLclVar()->SetMultiReg();
}
if (src->OperIs(GT_CALL))
{
lvaGetDesc(dest->AsLclVar())->lvIsMultiRegRet = true;
}
lvaGetDesc(dest->AsLclVar())->lvIsMultiRegRet = true;
}

dest->gtFlags |= destFlags;
Expand Down Expand Up @@ -17023,7 +17014,7 @@ GenTree* Compiler::impAssignMultiRegTypeToVar(GenTree* op,

assert(IsMultiRegReturnedType(hClass, callConv));

// Mark the var so that fields are not promoted and stay together.
// Set "lvIsMultiRegRet" to block promotion under "!lvaEnregMultiRegVars".
lvaTable[tmpNum].lvIsMultiRegRet = true;

return ret;
Expand Down
12 changes: 5 additions & 7 deletions src/coreclr/jit/lower.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3390,13 +3390,11 @@ void Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore)
DISPTREERANGE(BlockRange(), lclStore);
JITDUMP("\n");

GenTree* src = lclStore->gtGetOp1();
LclVarDsc* varDsc = comp->lvaGetDesc(lclStore);

GenTree* src = lclStore->gtGetOp1();
LclVarDsc* varDsc = comp->lvaGetDesc(lclStore);
const bool srcIsMultiReg = src->IsMultiRegNode();
const bool dstIsMultiReg = lclStore->IsMultiRegLclVar();

if (!dstIsMultiReg && varTypeIsStruct(varDsc))
if (!srcIsMultiReg && varTypeIsStruct(varDsc))
{
// TODO-Cleanup: we want to check `varDsc->lvRegStruct` as the last condition instead of `!varDsc->lvPromoted`,
// but we do not set it for `CSE` vars so it is currently failing.
Expand All @@ -3416,7 +3414,7 @@ void Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore)
}
}

if (srcIsMultiReg || dstIsMultiReg)
if (srcIsMultiReg)
{
const ReturnTypeDesc* retTypeDesc = nullptr;
if (src->OperIs(GT_CALL))
Expand Down Expand Up @@ -3577,7 +3575,7 @@ void Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore)
// src and dst can be in registers, check if we need a bitcast.
if (!src->TypeIs(TYP_STRUCT) && (varTypeUsesFloatReg(lclRegType) != varTypeUsesFloatReg(src)))
{
assert(!srcIsMultiReg && !dstIsMultiReg);
assert(!srcIsMultiReg);
assert(lclStore->OperIsLocalStore());
assert(lclRegType != TYP_UNDEF);

Expand Down
53 changes: 14 additions & 39 deletions src/coreclr/jit/morphblock.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ class MorphInitBlockHelper
FieldByField,
OneAsgBlock,
StructBlock,
SkipCallSrc,
SkipMultiRegIntrinsicSrc,
SkipMultiRegSrc,
SkipSingleRegCallSrc,
Nop
};

Expand Down Expand Up @@ -803,44 +803,25 @@ void MorphCopyBlockHelper::PrepareSrc()
// TrySpecialCases: check special cases that require special transformations.
// The current special cases include assignments with calls in RHS.
//
// Notes:
// It could change multiReg flags or change m_dst node.
//
void MorphCopyBlockHelper::TrySpecialCases()
{
#ifdef FEATURE_HW_INTRINSICS
if (m_src->OperIsHWIntrinsic() && HWIntrinsicInfo::IsMultiReg(m_src->AsHWIntrinsic()->GetHWIntrinsicId()))
if (m_src->IsMultiRegNode())
{
assert(m_src->IsMultiRegNode());
JITDUMP("Not morphing a multireg intrinsic\n");
m_transformationDecision = BlockTransformation::SkipMultiRegIntrinsicSrc;
m_result = m_asg;
}
#endif // FEATURE_HW_INTRINSICS
assert(m_dst->OperIs(GT_LCL_VAR));

#if FEATURE_MULTIREG_RET
// If this is a multi-reg return, we will not do any morphing of this node.
if (m_src->IsMultiRegCall())
{
assert(m_dst->OperGet() == GT_LCL_VAR);
JITDUMP("Not morphing a multireg call return\n");
m_transformationDecision = BlockTransformation::SkipCallSrc;
// This will exclude field locals (if any) from SSA: we do not have a way to
// associate multiple SSA definitions (SSA numbers) with one store.
m_dstVarDsc->lvIsMultiRegRet = true;

JITDUMP("Not morphing a multireg node return\n");
m_transformationDecision = BlockTransformation::SkipMultiRegSrc;
m_result = m_asg;
}
else if (m_dst->IsMultiRegLclVar() && !m_src->IsMultiRegNode())
else if (m_src->IsCall() && m_dst->OperIs(GT_LCL_VAR) && m_dstVarDsc->CanBeReplacedWithItsField(m_comp))
{
m_dst->AsLclVar()->ClearMultiReg();
}
#endif // FEATURE_MULTIREG_RET

if (m_transformationDecision == BlockTransformation::Undefined)
{
if (m_src->IsCall() && m_dst->OperIs(GT_LCL_VAR) && m_dstVarDsc->CanBeReplacedWithItsField(m_comp))
{
JITDUMP("Not morphing a single reg call return\n");
m_transformationDecision = BlockTransformation::SkipCallSrc;
m_result = m_asg;
}
JITDUMP("Not morphing a single reg call return\n");
m_transformationDecision = BlockTransformation::SkipSingleRegCallSrc;
m_result = m_asg;
}
}

Expand Down Expand Up @@ -1131,12 +1112,6 @@ void MorphCopyBlockHelper::MorphStructCases()
// Mark it as DoNotEnregister.
m_comp->lvaSetVarDoNotEnregister(m_dstLclNum DEBUGARG(DoNotEnregisterReason::BlockOp));
}
else if (m_dst->IsMultiRegLclVar())
{
// Handle this as lvIsMultiRegRet; this signals to SSA that it can't consider these fields
// SSA candidates (we don't have a way to represent multiple SSANums on MultiRegLclVar nodes).
m_dstVarDsc->lvIsMultiRegRet = true;
}
}

if (!m_srcDoFldAsg && (m_srcVarDsc != nullptr) && !m_srcSingleLclVarAsg)
Expand Down

0 comments on commit 0acd508

Please sign in to comment.