Skip to content

Commit

Permalink
JIT: Clean up gtCloneExpr (#97411)
Browse files Browse the repository at this point in the history 
- Remove its ability to replace locals by constants while cloning. Only
  loop unrolling uses this capability and it can be replaced by a simple
  visit over the tree after cloning, so that everyone else does not need
  to pay for it.
- Remove its ability to add flags. This is only used by hoisting to add
  `GTF_MAKE_CSE` flag recursively to the cloned tree. I do not see any
  good reason not just to only put this on the root node.
- Simplify how it propagates flags; simply copy them from the source
  node instead of having a separate `gtUpdateNodeSideEffects` call for
  the cloned tree. I do not see why the cloned version should have its
  flags updated like this when the original shouldn't.
  • Loading branch information
@jakobbotsch
jakobbotsch authored Jan 25, 2024
1 parent 2558ff9 commit 0626f2a
Show file tree
Hide file tree
Showing 5 changed files with 118 additions and 162 deletions.
7 changes: 2 additions & 5 deletions src/coreclr/jit/block.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -808,14 +808,11 @@ void* BasicBlock::MemoryPhiArg::operator new(size_t sz, Compiler* comp)
// compiler - Jit compiler instance
// to - New/empty block to copy statements into
// from - Block to copy statements from
// varNum - lclVar uses with lclNum `varNum` will be replaced; can be ~0 to indicate no replacement.
// varVal - If replacing uses of `varNum`, replace them with int constants with value `varVal`.
//
// Note:
// Leaves block ref count at zero, and pred edge list empty.
//
void BasicBlock::CloneBlockState(
Compiler* compiler, BasicBlock* to, const BasicBlock* from, unsigned varNum, int varVal)
void BasicBlock::CloneBlockState(Compiler* compiler, BasicBlock* to, const BasicBlock* from)
{
assert(to->bbStmtList == nullptr);
to->CopyFlags(from);
Expand All @@ -834,7 +831,7 @@ void BasicBlock::CloneBlockState(

for (Statement* const fromStmt : from->Statements())
{
GenTree* newExpr = compiler->gtCloneExpr(fromStmt->GetRootNode(), GTF_EMPTY, varNum, varVal);
GenTree* newExpr = compiler->gtCloneExpr(fromStmt->GetRootNode());
assert(newExpr != nullptr);
compiler->fgInsertStmtAtEnd(to, compiler->fgNewStmtFromTree(newExpr, fromStmt->GetDebugInfo()));
}
Expand Down
6 changes: 2 additions & 4 deletions src/coreclr/jit/block.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1672,10 +1672,8 @@ struct BasicBlock : private LIR::Range
return BBCompilerSuccList(comp, this);
}

// Clone block state and statements from `from` block to `to` block (which must be new/empty),
// optionally replacing uses of local `varNum` with IntCns `varVal`.
static void CloneBlockState(
Compiler* compiler, BasicBlock* to, const BasicBlock* from, unsigned varNum = (unsigned)-1, int varVal = 0);
// Clone block state and statements from `from` block to `to` block (which must be new/empty)
static void CloneBlockState(Compiler* compiler, BasicBlock* to, const BasicBlock* from);

// Copy the block kind and targets. The `from` block is untouched.
void CopyTarget(Compiler* compiler, const BasicBlock* from);
Expand Down
23 changes: 4 additions & 19 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3413,21 +3413,8 @@ class Compiler

GenTree* gtClone(GenTree* tree, bool complexOK = false);

// If `tree` is a lclVar with lclNum `varNum`, return an IntCns with value `varVal`; otherwise,
// create a copy of `tree`, adding specified flags, replacing uses of lclVar `deepVarNum` with
// IntCnses with value `deepVarVal`.
GenTree* gtCloneExpr(
GenTree* tree, GenTreeFlags addFlags, unsigned varNum, int varVal, unsigned deepVarNum, int deepVarVal);

// Create a copy of `tree`, optionally adding specified flags, and optionally mapping uses of local
// `varNum` to int constants with value `varVal`.
GenTree* gtCloneExpr(GenTree* tree,
GenTreeFlags addFlags = GTF_EMPTY,
unsigned varNum = BAD_VAR_NUM,
int varVal = 0)
{
return gtCloneExpr(tree, addFlags, varNum, varVal, varNum, varVal);
}
// Create a copy of `tree`
GenTree* gtCloneExpr(GenTree* tree);

Statement* gtCloneStmt(Statement* stmt)
{
Expand All @@ -3436,10 +3423,7 @@ class Compiler
}

// Internal helper for cloning a call
GenTreeCall* gtCloneExprCallHelper(GenTreeCall* call,
GenTreeFlags addFlags = GTF_EMPTY,
unsigned deepVarNum = BAD_VAR_NUM,
int deepVarVal = 0);
GenTreeCall* gtCloneExprCallHelper(GenTreeCall* call);

// Create copy of an inline or guarded devirtualization candidate tree.
GenTreeCall* gtCloneCandidateCall(GenTreeCall* call);
Expand Down Expand Up @@ -6801,6 +6785,7 @@ class Compiler
void optCloneLoop(FlowGraphNaturalLoop* loop, LoopCloneContext* context);
PhaseStatus optUnrollLoops(); // Unrolls loops (needs to have cost info)
bool optTryUnrollLoop(FlowGraphNaturalLoop* loop, bool* changedIR);
void optReplaceScalarUsesWithConst(BasicBlock* block, unsigned lclNum, ssize_t cnsVal);
void optRemoveRedundantZeroInits();
PhaseStatus optIfConversion(); // If conversion

Expand Down
170 changes: 40 additions & 130 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9214,29 +9214,15 @@ GenTree* Compiler::gtClone(GenTree* tree, bool complexOK)
}

//------------------------------------------------------------------------
// gtCloneExpr: Create a copy of `tree`, adding flags `addFlags`, mapping
// local `varNum` to int constant `varVal` if it appears at
// the root, and mapping uses of local `deepVarNum` to constant
// `deepVarVal` if they occur beyond the root.
// gtCloneExpr: Create a copy of `tree`
//
// Arguments:
// tree - GenTree to create a copy of
// addFlags - GTF_* flags to add to the copied tree nodes
// varNum - lclNum to replace at the root, or ~0 for no root replacement
// varVal - If replacing at root, replace local `varNum` with IntCns `varVal`
// deepVarNum - lclNum to replace uses of beyond the root, or ~0 for no replacement
// deepVarVal - If replacing beyond root, replace `deepVarNum` with IntCns `deepVarVal`
//
// Return Value:
// A copy of the given tree with the replacements and added flags specified.
//
// Notes:
// Top-level callers should generally call the overload that doesn't have
// the explicit `deepVarNum` and `deepVarVal` parameters; those are used in
// recursive invocations to avoid replacing defs.
// A copy of the given tree.

GenTree* Compiler::gtCloneExpr(
GenTree* tree, GenTreeFlags addFlags, unsigned varNum, int varVal, unsigned deepVarNum, int deepVarVal)
GenTree* Compiler::gtCloneExpr(GenTree* tree)
{
if (tree == nullptr)
{
Expand Down Expand Up @@ -9301,34 +9287,20 @@ GenTree* Compiler::gtCloneExpr(

case GT_LCL_VAR:

if (tree->AsLclVarCommon()->GetLclNum() == varNum)
{
copy = gtNewIconNode(varVal, tree->gtType);
}
else
{
// Remember that the local node has been cloned. The flag will be set on 'copy' as well.
tree->gtFlags |= GTF_VAR_MOREUSES;
copy = gtNewLclvNode(tree->AsLclVar()->GetLclNum(),
tree->gtType DEBUGARG(tree->AsLclVar()->gtLclILoffs));
copy->AsLclVarCommon()->SetSsaNum(tree->AsLclVarCommon()->GetSsaNum());
}
// Remember that the local node has been cloned. The flag will be set on 'copy' as well.
tree->gtFlags |= GTF_VAR_MOREUSES;
copy =
gtNewLclvNode(tree->AsLclVar()->GetLclNum(), tree->gtType DEBUGARG(tree->AsLclVar()->gtLclILoffs));
copy->AsLclVarCommon()->SetSsaNum(tree->AsLclVarCommon()->GetSsaNum());
goto DONE;

case GT_LCL_FLD:
if (tree->AsLclFld()->GetLclNum() == varNum)
{
IMPL_LIMITATION("replacing GT_LCL_FLD with a constant");
}
else
{
// Remember that the local node has been cloned. The flag will be set on 'copy' as well.
tree->gtFlags |= GTF_VAR_MOREUSES;
copy = new (this, GT_LCL_FLD)
GenTreeLclFld(GT_LCL_FLD, tree->TypeGet(), tree->AsLclFld()->GetLclNum(),
tree->AsLclFld()->GetLclOffs(), tree->AsLclFld()->GetLayout());
copy->AsLclFld()->SetSsaNum(tree->AsLclFld()->GetSsaNum());
}
// Remember that the local node has been cloned. The flag will be set on 'copy' as well.
tree->gtFlags |= GTF_VAR_MOREUSES;
copy =
new (this, GT_LCL_FLD) GenTreeLclFld(GT_LCL_FLD, tree->TypeGet(), tree->AsLclFld()->GetLclNum(),
tree->AsLclFld()->GetLclOffs(), tree->AsLclFld()->GetLayout());
copy->AsLclFld()->SetSsaNum(tree->AsLclFld()->GetSsaNum());
goto DONE;

case GT_RET_EXPR:
Expand Down Expand Up @@ -9582,38 +9554,14 @@ GenTree* Compiler::gtCloneExpr(
break;
}

// Some flags are conceptually part of the gtOper, and should be copied immediately.
if (tree->gtOverflowEx())
{
copy->gtFlags |= GTF_OVERFLOW;
}

if (tree->AsOp()->gtOp1)
{
copy->AsOp()->gtOp1 = gtCloneExpr(tree->AsOp()->gtOp1, addFlags, deepVarNum, deepVarVal);
copy->AsOp()->gtOp1 = gtCloneExpr(tree->AsOp()->gtOp1);
}

if (tree->gtGetOp2IfPresent())
{
copy->AsOp()->gtOp2 = gtCloneExpr(tree->AsOp()->gtOp2, addFlags, deepVarNum, deepVarVal);
}

/* Flags */
addFlags |= tree->gtFlags;

#ifdef DEBUG
/* GTF_NODE_MASK should not be propagated from 'tree' to 'copy' */
addFlags &= ~GTF_NODE_MASK;
#endif

// Effects flags propagate upwards.
if (copy->AsOp()->gtOp1 != nullptr)
{
copy->gtFlags |= (copy->AsOp()->gtOp1->gtFlags & GTF_ALL_EFFECT);
}
if (copy->gtGetOp2IfPresent() != nullptr)
{
copy->gtFlags |= (copy->gtGetOp2()->gtFlags & GTF_ALL_EFFECT);
copy->AsOp()->gtOp2 = gtCloneExpr(tree->AsOp()->gtOp2);
}

goto DONE;
Expand All @@ -9632,7 +9580,7 @@ GenTree* Compiler::gtCloneExpr(
NO_WAY("Cloning of calls with associated GT_RET_EXPR nodes is not supported");
}

copy = gtCloneExprCallHelper(tree->AsCall(), addFlags, deepVarNum, deepVarVal);
copy = gtCloneExprCallHelper(tree->AsCall());
break;

#ifdef FEATURE_HW_INTRINSICS
Expand All @@ -9648,7 +9596,7 @@ GenTree* Compiler::gtCloneExpr(
CLONE_MULTIOP_OPERANDS:
for (GenTree** use : copy->AsMultiOp()->UseEdges())
{
*use = gtCloneExpr(*use, addFlags, deepVarNum, deepVarVal);
*use = gtCloneExpr(*use);
}
break;
#endif
Expand All @@ -9659,11 +9607,10 @@ GenTree* Compiler::gtCloneExpr(
GenTree* inds[GT_ARR_MAX_RANK];
for (unsigned dim = 0; dim < arrElem->gtArrRank; dim++)
{
inds[dim] = gtCloneExpr(arrElem->gtArrInds[dim], addFlags, deepVarNum, deepVarVal);
inds[dim] = gtCloneExpr(arrElem->gtArrInds[dim]);
}
copy = new (this, GT_ARR_ELEM)
GenTreeArrElem(arrElem->TypeGet(), gtCloneExpr(arrElem->gtArrObj, addFlags, deepVarNum, deepVarVal),
arrElem->gtArrRank, arrElem->gtArrElemSize, &inds[0]);
copy = new (this, GT_ARR_ELEM) GenTreeArrElem(arrElem->TypeGet(), gtCloneExpr(arrElem->gtArrObj),
arrElem->gtArrRank, arrElem->gtArrElemSize, &inds[0]);
}
break;

Expand All @@ -9673,9 +9620,8 @@ GenTree* Compiler::gtCloneExpr(
GenTreePhi::Use** prevUse = &copy->AsPhi()->gtUses;
for (GenTreePhi::Use& use : tree->AsPhi()->Uses())
{
*prevUse = new (this, CMK_ASTNode)
GenTreePhi::Use(gtCloneExpr(use.GetNode(), addFlags, deepVarNum, deepVarVal), *prevUse);
prevUse = &((*prevUse)->NextRef());
*prevUse = new (this, CMK_ASTNode) GenTreePhi::Use(gtCloneExpr(use.GetNode()), *prevUse);
prevUse = &((*prevUse)->NextRef());
}
}
break;
Expand All @@ -9684,32 +9630,27 @@ GenTree* Compiler::gtCloneExpr(
copy = new (this, GT_FIELD_LIST) GenTreeFieldList();
for (GenTreeFieldList::Use& use : tree->AsFieldList()->Uses())
{
copy->AsFieldList()->AddField(this, gtCloneExpr(use.GetNode(), addFlags, deepVarNum, deepVarVal),
use.GetOffset(), use.GetType());
copy->AsFieldList()->AddField(this, gtCloneExpr(use.GetNode()), use.GetOffset(), use.GetType());
}
break;

case GT_CMPXCHG:
copy = new (this, GT_CMPXCHG)
GenTreeCmpXchg(tree->TypeGet(),
gtCloneExpr(tree->AsCmpXchg()->Addr(), addFlags, deepVarNum, deepVarVal),
gtCloneExpr(tree->AsCmpXchg()->Data(), addFlags, deepVarNum, deepVarVal),
gtCloneExpr(tree->AsCmpXchg()->Comparand(), addFlags, deepVarNum, deepVarVal));
GenTreeCmpXchg(tree->TypeGet(), gtCloneExpr(tree->AsCmpXchg()->Addr()),
gtCloneExpr(tree->AsCmpXchg()->Data()), gtCloneExpr(tree->AsCmpXchg()->Comparand()));
break;

case GT_STORE_DYN_BLK:
copy = new (this, oper)
GenTreeStoreDynBlk(gtCloneExpr(tree->AsStoreDynBlk()->Addr(), addFlags, deepVarNum, deepVarVal),
gtCloneExpr(tree->AsStoreDynBlk()->Data(), addFlags, deepVarNum, deepVarVal),
gtCloneExpr(tree->AsStoreDynBlk()->gtDynamicSize, addFlags, deepVarNum, deepVarVal));
copy = new (this, oper) GenTreeStoreDynBlk(gtCloneExpr(tree->AsStoreDynBlk()->Addr()),
gtCloneExpr(tree->AsStoreDynBlk()->Data()),
gtCloneExpr(tree->AsStoreDynBlk()->gtDynamicSize));
break;

case GT_SELECT:
copy = new (this, oper)
GenTreeConditional(oper, tree->TypeGet(),
gtCloneExpr(tree->AsConditional()->gtCond, addFlags, deepVarNum, deepVarVal),
gtCloneExpr(tree->AsConditional()->gtOp1, addFlags, deepVarNum, deepVarVal),
gtCloneExpr(tree->AsConditional()->gtOp2, addFlags, deepVarNum, deepVarVal));
copy =
new (this, oper) GenTreeConditional(oper, tree->TypeGet(), gtCloneExpr(tree->AsConditional()->gtCond),
gtCloneExpr(tree->AsConditional()->gtOp1),
gtCloneExpr(tree->AsConditional()->gtOp2));
break;
default:
#ifdef DEBUG
Expand All @@ -9719,33 +9660,10 @@ GenTree* Compiler::gtCloneExpr(
}

DONE:
assert(copy->gtOper == oper);

copy->gtVNPair = tree->gtVNPair; // A cloned tree gets the original's Value number pair

/* Compute the flags for the copied node. Note that we can do this only
if we didnt gtFoldExpr(copy) */

if (copy->gtOper == oper)
{
addFlags |= tree->gtFlags;

#ifdef DEBUG
/* GTF_NODE_MASK should not be propagated from 'tree' to 'copy' */
addFlags &= ~GTF_NODE_MASK;
#endif
copy->gtFlags |= addFlags;

// Update side effect flags since they may be different from the source side effect flags.
// For example, we may have replaced some locals with constants and made indirections non-throwing.
gtUpdateNodeSideEffects(copy);
if ((varNum != BAD_VAR_NUM) && copy->OperIsSsaDef())
{
fgAssignSetVarDef(copy);
}
}

/* GTF_COLON_COND should be propagated from 'tree' to 'copy' */
copy->gtFlags |= (tree->gtFlags & GTF_COLON_COND);
copy->gtFlags |= tree->gtFlags;

#if defined(DEBUG)
// Non-node debug flags should be propagated from 'tree' to 'copy'
Expand Down Expand Up @@ -9833,25 +9751,18 @@ void CallArgs::InternalCopyFrom(Compiler* comp, CallArgs* other, CopyNodeFunc co
//
// Arguments:
// tree - the call to clone
// addFlags - GTF_* flags to add to the copied tree nodes
// deepVarNum - lclNum to replace uses of beyond the root, or BAD_VAR_NUM for no replacement
// deepVarVal - If replacing beyond root, replace `deepVarNum` with IntCns `deepVarVal`
//
// Returns:
// Cloned copy of call and all subtrees.

GenTreeCall* Compiler::gtCloneExprCallHelper(GenTreeCall* tree,
GenTreeFlags addFlags,
unsigned deepVarNum,
int deepVarVal)
GenTreeCall* Compiler::gtCloneExprCallHelper(GenTreeCall* tree)
{
GenTreeCall* copy = new (this, GT_CALL) GenTreeCall(tree->TypeGet());

copy->gtCallMoreFlags = tree->gtCallMoreFlags;
INDEBUG(copy->gtCallDebugFlags = tree->gtCallDebugFlags);

copy->gtArgs.InternalCopyFrom(this, &tree->gtArgs,
[=](GenTree* node) { return gtCloneExpr(node, addFlags, deepVarNum, deepVarVal); });
copy->gtArgs.InternalCopyFrom(this, &tree->gtArgs, [=](GenTree* node) { return gtCloneExpr(node); });

// The call sig comes from the EE and doesn't change throughout the compilation process, meaning
// we only really need one physical copy of it. Therefore a shallow pointer copy will suffice.
Expand All @@ -9864,15 +9775,14 @@ GenTreeCall* Compiler::gtCloneExprCallHelper(GenTreeCall* tree,
copy->tailCallInfo = tree->tailCallInfo;

copy->gtRetClsHnd = tree->gtRetClsHnd;
copy->gtControlExpr = gtCloneExpr(tree->gtControlExpr, addFlags, deepVarNum, deepVarVal);
copy->gtControlExpr = gtCloneExpr(tree->gtControlExpr);
copy->gtStubCallStubAddr = tree->gtStubCallStubAddr;

/* Copy the union */
if (tree->gtCallType == CT_INDIRECT)
{
copy->gtCallCookie =
tree->gtCallCookie ? gtCloneExpr(tree->gtCallCookie, addFlags, deepVarNum, deepVarVal) : nullptr;
copy->gtCallAddr = tree->gtCallAddr ? gtCloneExpr(tree->gtCallAddr, addFlags, deepVarNum, deepVarVal) : nullptr;
copy->gtCallCookie = tree->gtCallCookie ? gtCloneExpr(tree->gtCallCookie) : nullptr;
copy->gtCallAddr = tree->gtCallAddr ? gtCloneExpr(tree->gtCallAddr) : nullptr;
}
else
{
Expand Down
Loading

0 comments on commit 0626f2a

Please sign in to comment.