diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 2f9abcb00703a..743cd5c4095fd 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -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, @@ -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 diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index d47c42a0ade52..bad626cf155da 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -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) { @@ -16636,7 +16636,6 @@ 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. @@ -16644,12 +16643,13 @@ bool Compiler::gtNodeHasSideEffects(GenTree* tree, GenTreeFlags flags) { // 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; } @@ -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; @@ -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; } @@ -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) @@ -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) @@ -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); diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index c2b186e691e89..9595be490519f 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -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 diff --git a/src/coreclr/jit/optcse.cpp b/src/coreclr/jit/optcse.cpp index b3d8e2610168a..4bd081e14ad7e 100644 --- a/src/coreclr/jit/optcse.cpp +++ b/src/coreclr/jit/optcse.cpp @@ -1878,7 +1878,7 @@ bool CSE_HeuristicCommon::CanConsiderTree(GenTree* tree, bool isReturn) // If we have a simple helper call with no other persistent side-effects // then we allow this tree to be a CSE candidate // - if (m_pCompiler->gtTreeHasSideEffects(tree, GTF_PERSISTENT_SIDE_EFFECTS | GTF_IS_IN_CSE)) + if (m_pCompiler->gtTreeHasSideEffects(tree, GTF_PERSISTENT_SIDE_EFFECTS, /* ignoreCctors */ true)) { return false; } @@ -5131,8 +5131,7 @@ void CSE_HeuristicCommon::PerformCSE(CSE_Candidate* successfulCandidate) // exp->gtCSEnum = NO_CSE; // clear the gtCSEnum field - GenTree* sideEffList = nullptr; - m_pCompiler->gtExtractSideEffList(exp, &sideEffList, GTF_PERSISTENT_SIDE_EFFECTS | GTF_IS_IN_CSE); + GenTree* sideEffList = m_pCompiler->optExtractSideEffectsForCSE(exp); // If we have any side effects or extracted CSE defs then we need to create a GT_COMMA tree instead // @@ -5414,6 +5413,113 @@ void CSE_HeuristicCommon::ConsiderCandidates() } } +//------------------------------------------------------------------------ +// optExtractSideEffectsForCSE: Extract side effects from a tree that is going +// to be CSE'd. This requires unmarking CSE uses and preserving CSE defs as if +// they were side effects. +// +// Parameters: +// tree - The tree containing side effects +// +// Return Value: +// Tree of side effects. +// +// Remarks: +// Unlike gtExtractSideEffList, this considers CSE defs to be side effects +// and also unmarks CSE uses as it proceeds. Additionally, for CSE we are ok +// with not treating cctor invocations as side effects because we have +// already handled those specially during CSE. +// +GenTree* Compiler::optExtractSideEffectsForCSE(GenTree* tree) +{ + class Extractor final : public GenTreeVisitor + { + GenTree* m_result = nullptr; + + public: + enum + { + DoPreOrder = true, + UseExecutionOrder = true + }; + + GenTree* GetResult() + { + return m_result; + } + + Extractor(Compiler* compiler) + : GenTreeVisitor(compiler) + { + } + + fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) + { + GenTree* node = *use; + + if (m_compiler->gtTreeHasSideEffects(node, GTF_PERSISTENT_SIDE_EFFECTS, /* ignoreCctors */ true)) + { + if (m_compiler->gtNodeHasSideEffects(node, GTF_PERSISTENT_SIDE_EFFECTS, /* ignoreCctors */ true)) + { + Append(node); + return Compiler::WALK_SKIP_SUBTREES; + } + + // 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()); + } + + // 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 (m_compiler->optUnmarkCSE(node)) + { + // The call to optUnmarkCSE(node) should have cleared any CSE info. + assert(!IS_CSE_INDEX(node->gtCSEnum)); + return Compiler::WALK_CONTINUE; + } + + 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 + Append(node); + return 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); + + // Set the ValueNumber 'gtVNPair' for the new GT_COMMA node + // + if ((m_compiler->vnStore != nullptr) && m_result->gtVNPair.BothDefined() && node->gtVNPair.BothDefined()) + { + ValueNumPair op1Exceptions = m_compiler->vnStore->VNPExceptionSet(m_result->gtVNPair); + comma->gtVNPair = m_compiler->vnStore->VNPWithExc(node->gtVNPair, op1Exceptions); + } + + m_result = comma; + } + }; + + Extractor extractor(this); + extractor.WalkTree(&tree, nullptr); + + return extractor.GetResult(); +} + //------------------------------------------------------------------------ // optValnumCSE_Heuristic: Perform common sub-expression elimination // based on profitabiliy heuristic