Skip to content

Commit

Permalink
JIT: Refactor gtExtractSideEffList (#79611)
Browse files Browse the repository at this point in the history 
Inline gtBuildCommaList (this is the only usage) and build it in the
right order to allow doing it during the walk. Also fix propagation of
GTF_DEBUG_NODE_MORPHED flag.

Fix #79543
@jakobbotsch
jakobbotsch authored Dec 15, 2022
1 parent d7924cc commit c0ebf2b
Showing 3 changed files with 73 additions and 91 deletions.
5 changes: 0 additions & 5 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
@@ -2800,11 +2800,6 @@ class Compiler
// Returns "true" iff "tree" or its (transitive) children have any of the side effects in "flags".
bool gtTreeHasSideEffects(GenTree* tree, GenTreeFlags flags);

// Appends 'expr' in front of 'list'
// 'list' will typically start off as 'nullptr'
// when 'list' is non-null a GT_COMMA node is used to insert 'expr'
GenTree* gtBuildCommaList(GenTree* list, GenTree* expr);

void gtExtractSideEffList(GenTree* expr,
GenTree** pList,
GenTreeFlags GenTreeFlags = GTF_SIDE_EFFECT,
153 changes: 73 additions & 80 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
@@ -16004,72 +16004,19 @@ bool Compiler::gtTreeHasSideEffects(GenTree* tree, GenTreeFlags flags /* = GTF_S
return true;
}

GenTree* Compiler::gtBuildCommaList(GenTree* list, GenTree* expr)
{
// 'list' starts off as null,
// and when it is null we haven't started the list yet.
//
if (list != nullptr)
{
// Create a GT_COMMA that appends 'expr' in front of the remaining set of expressions in (*list)
GenTree* result = gtNewOperNode(GT_COMMA, TYP_VOID, expr, list);

// Set the flags in the comma node
result->gtFlags |= (list->gtFlags & GTF_ALL_EFFECT);
result->gtFlags |= (expr->gtFlags & GTF_ALL_EFFECT);
DBEXEC(fgGlobalMorph, result->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);

// 'list' and 'expr' should have valuenumbers defined for both or for neither one (unless we are remorphing,
// in which case a prior transform involving either node may have discarded or otherwise invalidated the value
// numbers).
assert((list->gtVNPair.BothDefined() == expr->gtVNPair.BothDefined()) || !fgGlobalMorph);

// Set the ValueNumber 'gtVNPair' for the new GT_COMMA node
//
if (list->gtVNPair.BothDefined() && expr->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();

vnStore->VNPUnpackExc(expr->gtVNPair, &op1vnp, &op1Xvnp);
vnStore->VNPUnpackExc(list->gtVNPair, &op2vnp, &op2Xvnp);

ValueNumPair exceptions_vnp = ValueNumStore::VNPForEmptyExcSet();

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

result->gtVNPair = vnStore->VNPWithExc(op2vnp, exceptions_vnp);
}

return result;
}
else
{
// The 'expr' will start the list of expressions
return expr;
}
}

//------------------------------------------------------------------------
// gtExtractSideEffList: Extracts side effects from the given expression.
//
// Arguments:
// expr - the expression tree to extract side effects from
// pList - pointer to a (possibly null) GT_COMMA list that
// will contain the extracted side effects
// pList - pointer to a (possibly null) node
// flags - side effect flags to be considered
// ignoreRoot - ignore side effects on the expression root node
//
// Notes:
// Side effects are prepended to the GT_COMMA list such that op1 of
// each comma node holds the side effect tree and op2 points to the
// next comma node. The original side effect execution order is preserved.
// pList is modified such that the original pList is executed after all side
// effects that were extracted. The original side effect execution order is
// preserved.
//
void Compiler::gtExtractSideEffList(GenTree* expr,
GenTree** pList,
@@ -16078,18 +16025,23 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
{
class SideEffectExtractor final : public GenTreeVisitor<SideEffectExtractor>
{
public:
const GenTreeFlags m_flags;
ArrayStack<GenTree*> m_sideEffects;
const GenTreeFlags m_flags;

GenTree* m_result = nullptr;

public:
enum
{
DoPreOrder = true,
UseExecutionOrder = true
};

SideEffectExtractor(Compiler* compiler, GenTreeFlags flags)
: GenTreeVisitor(compiler), m_flags(flags), m_sideEffects(compiler->getAllocator(CMK_SideEffects))
GenTree* GetResult()
{
return m_result;
}

SideEffectExtractor(Compiler* compiler, GenTreeFlags flags) : GenTreeVisitor(compiler), m_flags(flags)
{
}

@@ -16103,7 +16055,7 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
{
if (m_compiler->gtNodeHasSideEffects(node, m_flags))
{
PushSideEffects(node);
Append(node);
if (node->OperIsBlk() && !node->OperIsStoreBlk())
{
JITDUMP("Replace an unused OBJ/BLK node [%06d] with a NULLCHECK\n", dspTreeID(node));
@@ -16120,7 +16072,7 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
// gtNodeHasSideEffects and make this check unconditionally.
if (node->OperIsAtomicOp())
{
PushSideEffects(node);
Append(node);
return Compiler::WALK_SKIP_SUBTREES;
}

@@ -16136,7 +16088,7 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
// those need to be extracted as if they're side effects.
if (!UnmarkCSE(node))
{
PushSideEffects(node);
Append(node);
return Compiler::WALK_SKIP_SUBTREES;
}

@@ -16148,6 +16100,59 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
return treeHasSideEffects ? Compiler::WALK_CONTINUE : Compiler::WALK_SKIP_SUBTREES;
}

void Append(GenTree* node)
{
if (m_result == nullptr)
{
m_result = node;
return;
}

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)
{
// Either both should be morphed or neither should be.
assert((m_result->gtDebugFlags & GTF_DEBUG_NODE_MORPHED) ==
(node->gtDebugFlags & GTF_DEBUG_NODE_MORPHED));
comma->gtDebugFlags |= node->gtDebugFlags & GTF_DEBUG_NODE_MORPHED;
}
#endif

// Both should have valuenumbers defined for both or for neither
// one (unless we are remorphing, in which case a prior transform
// involving either node may have discarded or otherwise
// invalidated the value numbers).
assert((m_result->gtVNPair.BothDefined() == node->gtVNPair.BothDefined()) || !m_compiler->fgGlobalMorph);

// Set the ValueNumber 'gtVNPair' for the new GT_COMMA node
//
if (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);
}

m_result = comma;
}

private:
bool UnmarkCSE(GenTree* node)
{
@@ -16172,11 +16177,6 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
return false;
}
}

void PushSideEffects(GenTree* node)
{
m_sideEffects.Push(node);
}
};

SideEffectExtractor extractor(this, flags);
@@ -16193,19 +16193,12 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
extractor.WalkTree(&expr, nullptr);
}

GenTree* list = *pList;

// The extractor returns side effects in execution order but gtBuildCommaList prepends
// to the comma-based side effect list so we have to build the list in reverse order.
// This is also why the list cannot be built while traversing the tree.
// The number of side effects is usually small (<= 4), less than the ArrayStack's
// built-in size, so memory allocation is avoided.
while (!extractor.m_sideEffects.Empty())
if (*pList != nullptr)
{
list = gtBuildCommaList(list, extractor.m_sideEffects.Pop());
extractor.Append(*pList);
}

*pList = list;
*pList = extractor.GetResult();
}

/*****************************************************************************
6 changes: 0 additions & 6 deletions src/coreclr/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
@@ -12100,12 +12100,6 @@ GenTree* Compiler::fgMorphModToZero(GenTreeOp* tree)
if (op1SideEffects != nullptr)
{
GenTree* comma = gtNewOperNode(GT_COMMA, zero->TypeGet(), op1SideEffects, zero);

#ifdef DEBUG
// op1 may already have been morphed, so unset this bit.
op1SideEffects->gtDebugFlags &= ~GTF_DEBUG_NODE_MORPHED;
#endif // DEBUG

INDEBUG(comma->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);

DEBUG_DESTROY_NODE(tree);

0 comments on commit c0ebf2b

Please sign in to comment.