Skip to content

Commit

Permalink
JIT: Remove CallArg::m_tmpNum (dotnet#104429)
Browse files Browse the repository at this point in the history 
Before this change the JIT has two places where it may introduce temps
during call args morphing:
1. Directly during `fgMorphArgs` as part of making struct copies for
   some struct args, like implicit byref args and other struct args
   requiring to be of `LCL_VAR` shape
2. During `EvalArgsToTemps` as part of making sure evaluation order is
   maintained while we get the call into the right shape with registers

To make this work we have `CallArg::m_tmpNum` that communicates the
local from 1 to 2; the responsibility of creating the actual `LCL_VAR`
node to put in the late argument list was left to `EvalArgsToTemps`
while `fgMorphArgs` would just change the early setup node to a store
into the temporary.

This PR changes it so that 1 directly introduces the right late node
when it creates its temporary. That is, it puts the call argument into
the right shape immediately and avoids relying on `EvalArgsToTemps` to
create the local node in the late list.

The benefit of that is that we no longer need to communicate the
temporary as part of every `CallArg`. However, the motivation here is
something else: as part of dotnet#104370 we may have arguments that need
multiple temporaries to copy, so having things working in this way
introduces complications for that.
  • Loading branch information
@jakobbotsch
jakobbotsch committed Jul 10, 2024
1 parent 812cb93 commit 7029008
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 119 deletions.
2 changes: 0 additions & 2 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9640,12 +9640,10 @@ void CallArgs::InternalCopyFrom(Compiler* comp, CallArgs* other, CopyNodeFunc co
carg->m_earlyNode = arg.m_earlyNode != nullptr ? copyNode(arg.m_earlyNode) : nullptr;
carg->m_lateNode = arg.m_lateNode != nullptr ? copyNode(arg.m_lateNode) : nullptr;
carg->m_signatureClsHnd = arg.m_signatureClsHnd;
carg->m_tmpNum = arg.m_tmpNum;
carg->m_signatureType = arg.m_signatureType;
carg->m_wellKnownArg = arg.m_wellKnownArg;
carg->m_needTmp = arg.m_needTmp;
carg->m_needPlace = arg.m_needPlace;
carg->m_isTmp = arg.m_isTmp;
carg->m_processed = arg.m_processed;
carg->AbiInfo = arg.AbiInfo;
carg->NewAbiInfo = arg.NewAbiInfo;
Expand Down
10 changes: 1 addition & 9 deletions src/coreclr/jit/gentree.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4708,8 +4708,6 @@ class CallArg

// The class handle for the signature type (when varTypeIsStruct(SignatureType)).
CORINFO_CLASS_HANDLE m_signatureClsHnd;
// The LclVar number if we had to force evaluation of this arg.
unsigned m_tmpNum;
// The type of the argument in the signature.
var_types m_signatureType : 5;
// The type of well-known argument this is.
Expand All @@ -4718,8 +4716,6 @@ class CallArg
bool m_needTmp : 1;
// True when we must replace this argument with a placeholder node.
bool m_needPlace : 1;
// True when we setup a temp LclVar for this argument.
bool m_isTmp : 1;
// True when we have decided the evaluation order for this argument in LateArgs
bool m_processed : 1;

Expand All @@ -4730,12 +4726,10 @@ class CallArg
, m_next(nullptr)
, m_lateNext(nullptr)
, m_signatureClsHnd(NO_CLASS_HANDLE)
, m_tmpNum(BAD_VAR_NUM)
, m_signatureType(TYP_UNDEF)
, m_wellKnownArg(WellKnownArg::None)
, m_needTmp(false)
, m_needPlace(false)
, m_isTmp(false)
, m_processed(false)
{
}
Expand Down Expand Up @@ -4772,7 +4766,6 @@ class CallArg
CORINFO_CLASS_HANDLE GetSignatureClassHandle() { return m_signatureClsHnd; }
var_types GetSignatureType() { return m_signatureType; }
WellKnownArg GetWellKnownArg() { return m_wellKnownArg; }
bool IsTemp() { return m_isTmp; }
// clang-format on

// Get the real argument node, i.e. not a setup or placeholder node.
Expand Down Expand Up @@ -4884,8 +4877,7 @@ class CallArgs
void SetNeedsTemp(CallArg* arg);
bool IsNonStandard(Compiler* comp, GenTreeCall* call, CallArg* arg);

GenTree* MakeTmpArgNode(Compiler* comp, CallArg* arg);
void SetTemp(CallArg* arg, unsigned tmpNum);
GenTree* MakeTmpArgNode(Compiler* comp, CallArg* arg, unsigned lclNum);

// clang-format off
bool HasThisPointer() const { return m_hasThisPointer; }
Expand Down
176 changes: 68 additions & 108 deletions src/coreclr/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -821,18 +821,10 @@ void CallArg::Dump(Compiler* comp)
{
printf(", isSplit");
}
if (m_needTmp)
{
printf(", tmpNum=V%02u", m_tmpNum);
}
if (m_needPlace)
{
printf(", needPlace");
}
if (m_isTmp)
{
printf(", isTmp");
}
if (m_processed)
{
printf(", processed");
Expand All @@ -849,15 +841,6 @@ void CallArg::Dump(Compiler* comp)
}
#endif

//------------------------------------------------------------------------
// SetTemp: Set that the specified argument was evaluated into a temp.
//
void CallArgs::SetTemp(CallArg* arg, unsigned tmpNum)
{
arg->m_tmpNum = tmpNum;
arg->m_isTmp = true;
}

//------------------------------------------------------------------------
// ArgsComplete: Make final decisions on which arguments to evaluate into temporaries.
//
Expand All @@ -874,13 +857,7 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call)
for (CallArg& arg : Args())
{
GenTree* argx = arg.GetEarlyNode();

if (argx == nullptr)
{
// Should only happen if remorphing in which case we do not need to
// make a decision about temps.
continue;
}
assert(argx != nullptr);

bool canEvalToTemp = true;
if (arg.AbiInfo.GetRegNum() == REG_STK)
Expand Down Expand Up @@ -909,19 +886,16 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call)
//
if ((argx->gtFlags & GTF_ASG) != 0)
{
// If this is not the only argument, or it's a copyblk, or it
// already evaluates the expression to a tmp then we need a temp in
// the late arg list.
// In the latter case this might not even be a value;
// fgMakeOutgoingStructArgCopy will leave the copying nodes here
// for FEATURE_FIXED_OUT_ARGS.
if (canEvalToTemp && ((argCount > 1) || argx->OperIsCopyBlkOp() || (FEATURE_FIXED_OUT_ARGS && arg.m_isTmp)))
// fgMakeOutgoingStructArgCopy can have introduced a temp already,
// in which case it will have created a setup node in the early
// node.
if (!argx->IsValue())
{
SetNeedsTemp(&arg);
assert(arg.m_needTmp);
}
else
else if (canEvalToTemp && (argCount > 1))
{
assert(argx->IsValue());
SetNeedsTemp(&arg);
}

// For all previous arguments that may interfere with the store we
Expand Down Expand Up @@ -1318,8 +1292,6 @@ void CallArgs::SortArgs(Compiler* comp, GenTreeCall* call, CallArg** sortedArgs)
regCount++;
}

assert(arg->GetLateNode() == nullptr);

// Skip any already processed args
//
if (!arg->m_processed)
Expand Down Expand Up @@ -1556,9 +1528,8 @@ void CallArgs::SortArgs(Compiler* comp, GenTreeCall* call, CallArg** sortedArgs)
// Return Value:
// the newly created temp var tree.
//
GenTree* CallArgs::MakeTmpArgNode(Compiler* comp, CallArg* arg)
GenTree* CallArgs::MakeTmpArgNode(Compiler* comp, CallArg* arg, unsigned lclNum)
{
unsigned lclNum = arg->m_tmpNum;
LclVarDsc* varDsc = comp->lvaGetDesc(lclNum);
var_types argType = varDsc->TypeGet();
assert(genActualType(argType) == genActualType(arg->GetSignatureType()));
Expand Down Expand Up @@ -1633,7 +1604,16 @@ void CallArgs::EvalArgsToTemps(Compiler* comp, GenTreeCall* call)
for (size_t i = 0; i < numArgs; i++)
{
CallArg& arg = *(sortedArgs[i]);
assert(arg.GetLateNode() == nullptr);

if (arg.GetLateNode() != nullptr)
{
// We may already have created the temp as part of
// fgMakeOutgoingStructArgCopy. In that case there is no work to be
// done.
*lateTail = &arg;
lateTail = &arg.LateNextRef();
continue;
}

GenTree* argx = arg.GetEarlyNode();
assert(argx != nullptr);
Expand All @@ -1655,82 +1635,60 @@ void CallArgs::EvalArgsToTemps(Compiler* comp, GenTreeCall* call)

if (arg.m_needTmp)
{
if (arg.m_isTmp)
{
// Create a copy of the temp to go into the late argument list
defArg = MakeTmpArgNode(comp, &arg);
}
else
{
// Create a temp store for the argument
// Put the temp in the late arg list
// Create a temp store for the argument
// Put the temp in the late arg list

#ifdef DEBUG
if (comp->verbose)
{
printf("Argument with 'side effect'...\n");
comp->gtDispTree(argx);
}
if (comp->verbose)
{
printf("Argument with 'side effect'...\n");
comp->gtDispTree(argx);
}
#endif

#if defined(TARGET_AMD64) && !defined(UNIX_AMD64_ABI)
noway_assert(argx->gtType != TYP_STRUCT);
noway_assert(argx->gtType != TYP_STRUCT);
#endif

unsigned tmpVarNum = comp->lvaGrabTemp(true DEBUGARG("argument with side effect"));
unsigned tmpVarNum = comp->lvaGrabTemp(true DEBUGARG("argument with side effect"));

if (setupArg != nullptr)
{
// Now keep the mkrefany for the late argument list
defArg = argx;
setupArg = comp->gtNewTempStore(tmpVarNum, argx);

// Clear the side-effect flags because now both op1 and op2 have no side-effects
defArg->gtFlags &= ~GTF_ALL_EFFECT;
}
else
{
setupArg = comp->gtNewTempStore(tmpVarNum, argx);
LclVarDsc* varDsc = comp->lvaGetDesc(tmpVarNum);
var_types lclVarType = genActualType(argx->gtType);
var_types scalarType = TYP_UNKNOWN;

LclVarDsc* varDsc = comp->lvaGetDesc(tmpVarNum);
var_types lclVarType = genActualType(argx->gtType);
var_types scalarType = TYP_UNKNOWN;

if (setupArg->OperIsCopyBlkOp())
{
setupArg = comp->fgMorphCopyBlock(setupArg);
if (setupArg->OperIsCopyBlkOp())
{
setupArg = comp->fgMorphCopyBlock(setupArg);
#if defined(TARGET_ARMARCH) || defined(UNIX_AMD64_ABI) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64)
if ((lclVarType == TYP_STRUCT) && (arg.AbiInfo.ArgType != TYP_STRUCT))
{
scalarType = arg.AbiInfo.ArgType;
}
if ((lclVarType == TYP_STRUCT) && (arg.AbiInfo.ArgType != TYP_STRUCT))
{
scalarType = arg.AbiInfo.ArgType;
}
#endif // TARGET_ARMARCH || defined (UNIX_AMD64_ABI) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64)
}

// scalarType can be set to a wider type for ARM or unix amd64 architectures: (3 => 4) or (5,6,7 =>
// 8)
if ((scalarType != TYP_UNKNOWN) && (scalarType != lclVarType))
{
// Create a GT_LCL_FLD using the wider type to go to the late argument list
defArg = comp->gtNewLclFldNode(tmpVarNum, scalarType, 0);
}
else
{
// Create a copy of the temp to go to the late argument list
defArg = comp->gtNewLclvNode(tmpVarNum, lclVarType);
}
}

arg.m_isTmp = true;
arg.m_tmpNum = tmpVarNum;
}
// scalarType can be set to a wider type for ARM or unix amd64 architectures: (3 => 4) or (5,6,7 =>
// 8)
if ((scalarType != TYP_UNKNOWN) && (scalarType != lclVarType))
{
// Create a GT_LCL_FLD using the wider type to go to the late argument list
defArg = comp->gtNewLclFldNode(tmpVarNum, scalarType, 0);
}
else
{
// Create a copy of the temp to go to the late argument list
defArg = comp->gtNewLclvNode(tmpVarNum, lclVarType);
}

#ifdef DEBUG
if (comp->verbose)
{
printf("\n Evaluate to a temp:\n");
comp->gtDispTree(setupArg);
}
#endif
if (comp->verbose)
{
printf("\n Evaluate to a temp:\n");
comp->gtDispTree(setupArg);
}
#endif
}
else // curArgTabEntry->needTmp == false
{
Expand Down Expand Up @@ -3289,29 +3247,31 @@ void Compiler::fgMakeOutgoingStructArgCopy(GenTreeCall* call, CallArg* arg)
assert(!fgGlobalMorph);
}

call->gtArgs.SetNeedsTemp(arg);

// Copy the valuetype to the temp
GenTree* copyBlk = gtNewStoreLclVarNode(tmp, argx);
copyBlk = fgMorphCopyBlock(copyBlk);

call->gtArgs.SetTemp(arg, tmp);
#if FEATURE_FIXED_OUT_ARGS

// Do the copy early, and evaluate the temp later (see EvalArgsToTemps)
// When on Unix create LCL_FLD for structs passed in more than one registers. See fgMakeTmpArgNode
GenTree* argNode = copyBlk;
// For fixed out args we create the setup node here; EvalArgsToTemps knows
// to handle the case of "already have a setup node" properly.
arg->SetEarlyNode(copyBlk);
arg->SetLateNode(call->gtArgs.MakeTmpArgNode(this, arg, tmp));

#else // !FEATURE_FIXED_OUT_ARGS

// Structs are always on the stack, and thus never need temps
// so we have to put the copy and temp all into one expression.
GenTree* argNode = call->gtArgs.MakeTmpArgNode(this, arg);
GenTree* argNode = call->gtArgs.MakeTmpArgNode(this, arg, tmp);

// Change the expression to "(tmp=val),tmp"
argNode = gtNewOperNode(GT_COMMA, argNode->TypeGet(), copyBlk, argNode);

#endif // !FEATURE_FIXED_OUT_ARGS

arg->SetEarlyNode(argNode);

#endif // !FEATURE_FIXED_OUT_ARGS
}

/*****************************************************************************
Expand Down Expand Up @@ -6772,12 +6732,12 @@ Statement* Compiler::fgAssignRecursiveCallArgToCallerParam(GenTree* arg,
// TODO-CQ: enable calls with struct arguments passed in registers.
noway_assert(!varTypeIsStruct(arg->TypeGet()));

if (callArg->IsTemp() || arg->IsCnsIntOrI() || arg->IsCnsFltOrDbl())
if (arg->IsCnsIntOrI() || arg->IsCnsFltOrDbl())
{
// The argument is already assigned to a temp or is a const.
argInTemp = arg;
}
else if (arg->OperGet() == GT_LCL_VAR)
else if (arg->OperIs(GT_LCL_VAR))
{
unsigned lclNum = arg->AsLclVar()->GetLclNum();
LclVarDsc* varDsc = lvaGetDesc(lclNum);
Expand Down

0 comments on commit 7029008

Please sign in to comment.