Skip to content

JIT: Remove GTF_IS_IN_CSE #104855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 15, 2024
Merged

JIT: Remove GTF_IS_IN_CSE #104855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3770c5b
JIT: Remove `GTF_IS_IN_CSE`
jakobbotsch Jul 13, 2024
5b5b849
Nit
jakobbotsch Jul 13, 2024
e087cdf
Merge branch 'main' of github.com:dotnet/runtime into remove-GTF_IS_I…
jakobbotsch Jul 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
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 @@ -7248,6 +7248,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
Open in desktop
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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gtNewOperNode does the flags propagation already.


#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
Open in desktop
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