diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 70432157420ffb..d19e1ee8b92fa9 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -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, diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 41f60a4301bb87..eef695b7b9d8b6 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -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, @@ -16079,18 +16026,23 @@ void Compiler::gtExtractSideEffList(GenTree* expr, { class SideEffectExtractor final : public GenTreeVisitor { - public: - const GenTreeFlags m_flags; - ArrayStack 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) { } @@ -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)); @@ -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; } @@ -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; } @@ -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; + } +#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) { @@ -16173,11 +16178,6 @@ void Compiler::gtExtractSideEffList(GenTree* expr, return false; } } - - void PushSideEffects(GenTree* node) - { - m_sideEffects.Push(node); - } }; SideEffectExtractor extractor(this, flags); @@ -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(); } /***************************************************************************** diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index f0ff9b27c8b88d..1acf8dcab8eae7 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -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);