Skip to content

JIT: Refactor gtExtractSideEffList and fix GTF_DEBUG_NODE_MORPHED propagation #79611

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
jakobbotsch merged 2 commits into from
Dec 15, 2022
Merged

JIT: Refactor gtExtractSideEffList and fix GTF_DEBUG_NODE_MORPHED propagation #79611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39cd183
JIT: Refactor gtExtractSideEffList
jakobbotsch Dec 13, 2022
1b2017c
Merge branch 'main' of github.com:dotnet/runtime into refactor-gtExtr…
jakobbotsch Dec 15, 2022
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: 0 additions & 5 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
153 changes: 73 additions & 80 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16005,72 +16005,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,
Expand All @@ -16079,18 +16026,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)
{
}

Expand All @@ -16104,7 +16056,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));
Expand All @@ -16121,7 +16073,7 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
// gtNodeHasSideEffects and make this check unconditionally.
if (node->OperIsAtomicOp())
{
PushSideEffects(node);
Append(node);
return Compiler::WALK_SKIP_SUBTREES;
}

Expand All @@ -16137,7 +16089,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;
}

Expand All @@ -16149,6 +16101,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;
Comment on lines +16118 to +16121
Copy link
Copy Markdown
Member Author

@jakobbotsch jakobbotsch Dec 13, 2022

Choose a reason for hiding this comment

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

This is the fix for #79543

}
#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)
{
Expand All @@ -16173,11 +16178,6 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
return false;
}
}

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

SideEffectExtractor extractor(this, flags);
Expand All @@ -16194,19 +16194,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();
}

/*****************************************************************************
Expand Down
6 changes: 0 additions & 6 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12090,12 +12090,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);
Expand Down