Skip to content

Commit

Permalink
JIT: Remove GTF_IS_IN_CSE (dotnet#104855)
Browse files Browse the repository at this point in the history 
This flag is just a convoluted way to pass an argument through to a
bunch of methods from inside CSE. That's because CSE tries to reuse
`gtExtractSideEffList` even though it needs something more capable that
considers CSE defs and CSE uses as well.

Remove the flag in favor of an `ignoreCctors` flag in the side effect
checking functions; then, additionally add a CSE-specific version of
`gtExtractSideEffList` called `optExtractSideEffectsForCSE` which
handles side effects and also CSE defs/uses. This does result in a
slight amount of duplication, but I think that's beneficial over the
convoluted logic before.
  • Loading branch information
@jakobbotsch
jakobbotsch authored Jul 15, 2024
1 parent 5354e0f commit a86987c
Show file tree
Hide file tree
Showing 4 changed files with 161 additions and 124 deletions.
5 changes: 3 additions & 2 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3648,10 +3648,10 @@ class Compiler
void gtSetStmtInfo(Statement* stmt);

// Returns "true" iff "node" has any of the side effects in "flags".
bool gtNodeHasSideEffects(GenTree* node, GenTreeFlags flags);
bool gtNodeHasSideEffects(GenTree* node, GenTreeFlags flags, bool ignoreCctors = false);

// Returns "true" iff "tree" or its (transitive) children have any of the side effects in "flags".
bool gtTreeHasSideEffects(GenTree* tree, GenTreeFlags flags);
bool gtTreeHasSideEffects(GenTree* tree, GenTreeFlags flags, bool ignoreCctors = false);

void gtExtractSideEffList(GenTree* expr,
GenTree** pList,
Expand Down Expand Up @@ -7251,6 +7251,7 @@ class Compiler
void optValnumCSE_DataFlow();
void optValnumCSE_Availability();
void optValnumCSE_Heuristic(CSE_HeuristicCommon* heuristic);
GenTree* optExtractSideEffectsForCSE(GenTree* tree);

bool optDoCSE; // True when we have found a duplicate CSE tree
bool optValnumCSE_phase; // True when we are executing the optOptimizeValnumCSEs() phase
Expand Down
159 changes: 49 additions & 110 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16609,7 +16609,7 @@ GenTree* Compiler::gtNewRefCOMfield(GenTree* objPtr,
* It may return false even if the node has GTF_SIDE_EFFECT (because of its children).
*/

bool Compiler::gtNodeHasSideEffects(GenTree* tree, GenTreeFlags flags)
bool Compiler::gtNodeHasSideEffects(GenTree* tree, GenTreeFlags flags, bool ignoreCctors)
{
if (flags & GTF_ASG)
{
Expand All @@ -16636,20 +16636,20 @@ bool Compiler::gtNodeHasSideEffects(GenTree* tree, GenTreeFlags flags)
{
GenTreeCall* const call = potentialCall->AsCall();
const bool ignoreExceptions = (flags & GTF_EXCEPT) == 0;
const bool ignoreCctors = (flags & GTF_IS_IN_CSE) != 0; // We can CSE helpers that run cctors.
if (!call->HasSideEffects(this, ignoreExceptions, ignoreCctors))
{
// If this call is otherwise side effect free, check its arguments.
for (CallArg& arg : call->gtArgs.Args())
{
// I'm a little worried that args that assign to temps that are late args will look like
// side effects...but better to be conservative for now.
if ((arg.GetEarlyNode() != nullptr) && gtTreeHasSideEffects(arg.GetEarlyNode(), flags))
if ((arg.GetEarlyNode() != nullptr) &&
gtTreeHasSideEffects(arg.GetEarlyNode(), flags, ignoreCctors))
{
return true;
}

if ((arg.GetLateNode() != nullptr) && gtTreeHasSideEffects(arg.GetLateNode(), flags))
if ((arg.GetLateNode() != nullptr) && gtTreeHasSideEffects(arg.GetLateNode(), flags, ignoreCctors))
{
return true;
}
Expand Down Expand Up @@ -16686,7 +16686,7 @@ bool Compiler::gtNodeHasSideEffects(GenTree* tree, GenTreeFlags flags)
* Returns true if the expr tree has any side effects.
*/

bool Compiler::gtTreeHasSideEffects(GenTree* tree, GenTreeFlags flags /* = GTF_SIDE_EFFECT*/)
bool Compiler::gtTreeHasSideEffects(GenTree* tree, GenTreeFlags flags /* = GTF_SIDE_EFFECT*/, bool ignoreCctors)
{
// These are the side effect flags that we care about for this tree
GenTreeFlags sideEffectFlags = tree->gtFlags & flags;
Expand All @@ -16709,22 +16709,22 @@ bool Compiler::gtTreeHasSideEffects(GenTree* tree, GenTreeFlags flags /* = GTF_S
// If this node is a helper call we may not care about the side-effects.
// Note that gtNodeHasSideEffects checks the side effects of the helper itself
// as well as the side effects of its arguments.
return gtNodeHasSideEffects(tree, flags);
return gtNodeHasSideEffects(tree, flags, ignoreCctors);
}
}
else if (tree->OperGet() == GT_INTRINSIC)
{
if (gtNodeHasSideEffects(tree, flags))
if (gtNodeHasSideEffects(tree, flags, ignoreCctors))
{
return true;
}

if (gtNodeHasSideEffects(tree->AsOp()->gtOp1, flags))
if (gtNodeHasSideEffects(tree->AsOp()->gtOp1, flags, ignoreCctors))
{
return true;
}

if ((tree->AsOp()->gtOp2 != nullptr) && gtNodeHasSideEffects(tree->AsOp()->gtOp2, flags))
if ((tree->AsOp()->gtOp2 != nullptr) && gtNodeHasSideEffects(tree->AsOp()->gtOp2, flags, ignoreCctors))
{
return true;
}
Expand Down Expand Up @@ -17110,81 +17110,61 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
{
GenTree* node = *use;

bool treeHasSideEffects = m_compiler->gtTreeHasSideEffects(node, m_flags);
if (!m_compiler->gtTreeHasSideEffects(node, m_flags))
{
return Compiler::WALK_SKIP_SUBTREES;
}

if (treeHasSideEffects)
if (m_compiler->gtNodeHasSideEffects(node, m_flags))
{
if (m_compiler->gtNodeHasSideEffects(node, m_flags))
if (node->OperIsBlk() && !node->OperIsStoreBlk())
{
if (node->OperIsBlk() && !node->OperIsStoreBlk())
{
JITDUMP("Replace an unused BLK node [%06d] with a NULLCHECK\n", dspTreeID(node));
m_compiler->gtChangeOperToNullCheck(node, m_compiler->compCurBB);
}

Append(node);
return Compiler::WALK_SKIP_SUBTREES;
JITDUMP("Replace an unused BLK node [%06d] with a NULLCHECK\n", dspTreeID(node));
m_compiler->gtChangeOperToNullCheck(node, m_compiler->compCurBB);
}

if (node->OperIs(GT_QMARK))
{
GenTree* prevSideEffects = m_result;
// Visit children out of order so we know if we can
// completely remove the qmark. We cannot modify the
// condition if we cannot completely remove the qmark, so
// we cannot visit it first.
Append(node);
return Compiler::WALK_SKIP_SUBTREES;
}

GenTreeQmark* qmark = node->AsQmark();
GenTreeColon* colon = qmark->gtGetOp2()->AsColon();
if (node->OperIs(GT_QMARK))
{
GenTree* prevSideEffects = m_result;
// Visit children out of order so we know if we can
// completely remove the qmark. We cannot modify the
// condition if we cannot completely remove the qmark, so
// we cannot visit it first.

m_result = nullptr;
WalkTree(&colon->gtOp1, colon);
GenTree* thenSideEffects = m_result;
GenTreeQmark* qmark = node->AsQmark();
GenTreeColon* colon = qmark->gtGetOp2()->AsColon();

m_result = nullptr;
WalkTree(&colon->gtOp2, colon);
GenTree* elseSideEffects = m_result;
m_result = nullptr;
WalkTree(&colon->gtOp1, colon);
GenTree* thenSideEffects = m_result;

m_result = prevSideEffects;
m_result = nullptr;
WalkTree(&colon->gtOp2, colon);
GenTree* elseSideEffects = m_result;

if ((thenSideEffects == nullptr) && (elseSideEffects == nullptr))
{
WalkTree(&qmark->gtOp1, qmark);
}
else
{
colon->gtOp1 = (thenSideEffects != nullptr) ? thenSideEffects : m_compiler->gtNewNothingNode();
colon->gtOp2 = (elseSideEffects != nullptr) ? elseSideEffects : m_compiler->gtNewNothingNode();
qmark->gtType = TYP_VOID;
colon->gtType = TYP_VOID;
Append(qmark);
}
m_result = prevSideEffects;

return Compiler::WALK_SKIP_SUBTREES;
if ((thenSideEffects == nullptr) && (elseSideEffects == nullptr))
{
WalkTree(&qmark->gtOp1, qmark);
}

// Generally all GT_CALL nodes are considered to have side-effects.
// So if we get here it must be a helper call that we decided it does
// not have side effects that we needed to keep.
assert(!node->OperIs(GT_CALL) || node->AsCall()->IsHelperCall());
}

if ((m_flags & GTF_IS_IN_CSE) != 0)
{
// If we're doing CSE then we also need to unmark CSE nodes. This will fail for CSE defs,
// those need to be extracted as if they're side effects.
if (!UnmarkCSE(node))
else
{
Append(node);
return Compiler::WALK_SKIP_SUBTREES;
colon->gtOp1 = (thenSideEffects != nullptr) ? thenSideEffects : m_compiler->gtNewNothingNode();
colon->gtOp2 = (elseSideEffects != nullptr) ? elseSideEffects : m_compiler->gtNewNothingNode();
qmark->gtType = TYP_VOID;
colon->gtType = TYP_VOID;
Append(qmark);
}

// The existence of CSE defs and uses is not propagated up the tree like side
// effects are. We need to continue visiting the tree as if it has side effects.
treeHasSideEffects = true;
return Compiler::WALK_SKIP_SUBTREES;
}

return treeHasSideEffects ? Compiler::WALK_CONTINUE : Compiler::WALK_SKIP_SUBTREES;
return Compiler::WALK_CONTINUE;
}

void Append(GenTree* node)
Expand All @@ -17196,7 +17176,6 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
}

GenTree* comma = m_compiler->gtNewOperNode(GT_COMMA, TYP_VOID, m_result, node);
comma->gtFlags |= (m_result->gtFlags | node->gtFlags) & GTF_ALL_EFFECT;

#ifdef DEBUG
if (m_compiler->fgGlobalMorph)
Expand All @@ -17218,52 +17197,12 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
//
if ((m_compiler->vnStore != nullptr) && m_result->gtVNPair.BothDefined() && node->gtVNPair.BothDefined())
{
// The result of a GT_COMMA node is op2, the normal value number is op2vnp
// But we also need to include the union of side effects from op1 and op2.
// we compute this value into exceptions_vnp.
ValueNumPair op1vnp;
ValueNumPair op1Xvnp = ValueNumStore::VNPForEmptyExcSet();
ValueNumPair op2vnp;
ValueNumPair op2Xvnp = ValueNumStore::VNPForEmptyExcSet();

m_compiler->vnStore->VNPUnpackExc(node->gtVNPair, &op1vnp, &op1Xvnp);
m_compiler->vnStore->VNPUnpackExc(m_result->gtVNPair, &op2vnp, &op2Xvnp);

ValueNumPair exceptions_vnp = ValueNumStore::VNPForEmptyExcSet();

exceptions_vnp = m_compiler->vnStore->VNPExcSetUnion(exceptions_vnp, op1Xvnp);
exceptions_vnp = m_compiler->vnStore->VNPExcSetUnion(exceptions_vnp, op2Xvnp);

comma->gtVNPair = m_compiler->vnStore->VNPWithExc(op2vnp, exceptions_vnp);
ValueNumPair op1Exceptions = m_compiler->vnStore->VNPExceptionSet(m_result->gtVNPair);
comma->gtVNPair = m_compiler->vnStore->VNPWithExc(node->gtVNPair, op1Exceptions);
}

m_result = comma;
}

private:
bool UnmarkCSE(GenTree* node)
{
assert(m_compiler->optValnumCSE_phase);

if (m_compiler->optUnmarkCSE(node))
{
// The call to optUnmarkCSE(node) should have cleared any CSE info.
assert(!IS_CSE_INDEX(node->gtCSEnum));
return true;
}
else
{
assert(IS_CSE_DEF(node->gtCSEnum));
#ifdef DEBUG
if (m_compiler->verbose)
{
printf("Preserving the CSE def #%02d at ", GET_CSE_INDEX(node->gtCSEnum));
m_compiler->printTreeID(node);
}
#endif
return false;
}
}
};

SideEffectExtractor extractor(this, flags);
Expand Down
9 changes: 0 additions & 9 deletions src/coreclr/jit/gentree.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -404,15 +404,6 @@ enum GenTreeFlags : unsigned int
// With operators: the specified node is an unsigned operator
GTF_SPILL = 0x00020000, // Needs to be spilled here

// The extra flag GTF_IS_IN_CSE is used to tell the consumer of the side effect flags
// that we are calling in the context of performing a CSE, thus we
// should allow the run-once side effects of running a class constructor.
//
// The only requirement of this flag is that it not overlap any of the
// side-effect flags. The actual bit used is otherwise arbitrary.

GTF_IS_IN_CSE = 0x00004000,

GTF_COMMON_MASK = 0x0003FFFF, // mask of all the flags above

GTF_REUSE_REG_VAL = 0x00800000, // This is set by the register allocator on nodes whose value already exists in the
Expand Down
Loading

0 comments on commit a86987c

Please sign in to comment.