From 030c7cf4602ee9d5caef6104d2fac02785d62b9c Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 23 Jan 2024 19:46:51 +0100 Subject: [PATCH 1/7] JIT: Improve call args interference checking when stores are involved - Fix the propagation of `GTF_ASG` during call args morphing - Introduce `Compiler::gtHaveStoreInterference` that can check whether two trees interfere with each other due to a store in one tree that stores to a local read by the other tree - Use the new helper when checking for whether we should reverse `GTF_STOREIND` nodes - Use the new helper when deciding whether previous args need to be evaluated to temps because we see an argument with an embedded store (typically a call). Fix #13758 --- src/coreclr/jit/compiler.h | 2 + src/coreclr/jit/fgdiagnostic.cpp | 15 ---- src/coreclr/jit/gentree.cpp | 121 ++++++++++++++++++++++++++++++- src/coreclr/jit/morph.cpp | 44 ++++++----- 4 files changed, 149 insertions(+), 33 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index d5b465d6dfa821..319bc836dd589f 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3487,6 +3487,8 @@ class Compiler bool gtMarkAddrMode(GenTree* addr, int* costEx, int* costSz, var_types type); unsigned gtSetEvalOrder(GenTree* tree); + bool gtHaveStoreInterference(GenTree* treeWithStores, GenTree* tree); + bool gtTreeHasLocalRead(GenTree* tree, unsigned lclNum); void gtSetStmtInfo(Statement* stmt); diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index f1f1afd357bced..86f7f3ecee8de9 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -3350,21 +3350,6 @@ void Compiler::fgDebugCheckFlags(GenTree* tree, BasicBlock* block) assert(doesMethodHaveRecursiveTailcall()); assert(block->HasFlag(BBF_RECURSIVE_TAILCALL)); } - - for (CallArg& arg : call->gtArgs.Args()) - { - // TODO-Cleanup: this is a patch for a violation in our GTF_ASG propagation. - // see https://github.com/dotnet/runtime/issues/13758 - if (arg.GetEarlyNode() != nullptr) - { - actualFlags |= arg.GetEarlyNode()->gtFlags & GTF_ASG; - } - - if (arg.GetLateNode() != nullptr) - { - actualFlags |= arg.GetLateNode()->gtFlags & GTF_ASG; - } - } } break; diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index ac88ed8abae8b9..6c52595a6d2b5a 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -5935,7 +5935,7 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) } // In case op2 assigns to a local var that is used in op1, we have to evaluate op1 first. - if ((op2->gtFlags & GTF_ASG) != 0) + if (gtHaveStoreInterference(op2, op1)) { // TODO-ASG-Cleanup: move this guard to "gtCanSwapOrder". allowReversal = false; @@ -6317,6 +6317,125 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) #pragma warning(pop) #endif +//------------------------------------------------------------------------ +// gtHaveStoreInterference: Check if two trees interfere because of a store in +// one of the trees. +// +// Parameters: +// treeWithStores - Tree that may have stores in it +// tree - Tree that may be reading from a store in "treeWithStores" +// +// Returns: +// True if there is any GT_LCL_VAR or GT_LCL_FLD node whose value depends on +// "lclNum". +// +bool Compiler::gtHaveStoreInterference(GenTree* treeWithStores, GenTree* tree) +{ + if (((treeWithStores->gtFlags & GTF_ASG) == 0) || tree->IsInvariant()) + { + return false; + } + + class Visitor final : public GenTreeVisitor + { + GenTree* m_op1; + + public: + enum + { + DoPreOrder = true, + }; + + Visitor(Compiler* compiler, GenTree* op1) : GenTreeVisitor(compiler), m_op1(op1) + { + } + + Compiler::fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) + { + GenTree* node = *use; + if ((node->gtFlags & GTF_ASG) == 0) + { + return WALK_SKIP_SUBTREES; + } + + if (node->OperIsLocalStore()) + { + if (m_compiler->gtTreeHasLocalRead(m_op1, node->AsLclVarCommon()->GetLclNum())) + { + return WALK_ABORT; + } + } + + return WALK_CONTINUE; + } + }; + + Visitor visitor(this, tree); + return visitor.WalkTree(&treeWithStores, nullptr) == WALK_ABORT; +} + +//------------------------------------------------------------------------ +// gtTreeHasLocalRead: Check if a tree has a read of the specified local, +// taking promotion into account. +// +// Parameters: +// tree - The tree to check. +// lclNum - The local to look for. +// +// Returns: +// True if there is any GT_LCL_VAR or GT_LCL_FLD node whose value depends on +// "lclNum". +// +bool Compiler::gtTreeHasLocalRead(GenTree* tree, unsigned lclNum) +{ + class Visitor final : public GenTreeVisitor + { + public: + enum + { + DoPreOrder = true, + DoLclVarsOnly = true, + }; + + unsigned m_lclNum; + LclVarDsc* m_lclDsc; + + Visitor(Compiler* compiler, unsigned lclNum) : GenTreeVisitor(compiler), m_lclNum(lclNum) + { + m_lclDsc = compiler->lvaGetDesc(lclNum); + } + + Compiler::fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) + { + GenTree* node = *use; + + if (node->OperIsLocalRead()) + { + if (node->AsLclVarCommon()->GetLclNum() == m_lclNum) + { + return WALK_ABORT; + } + + if (m_lclDsc->lvIsStructField && (node->AsLclVarCommon()->GetLclNum() == m_lclDsc->lvParentLcl)) + { + return WALK_ABORT; + } + + if (m_lclDsc->lvPromoted && (node->AsLclVarCommon()->GetLclNum() >= m_lclDsc->lvFieldLclStart) && + (node->AsLclVarCommon()->GetLclNum() < m_lclDsc->lvFieldLclStart + m_lclDsc->lvFieldCnt)) + { + return WALK_ABORT; + } + } + + return WALK_CONTINUE; + } + }; + + Visitor visitor(this, lclNum); + return visitor.WalkTree(&tree, nullptr) == WALK_ABORT; +} + #ifdef DEBUG bool GenTree::OperSupportsReverseOpEvalOrder(Compiler* comp) const { diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 33428d5fd5a11f..cedc7628f716ca 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -912,20 +912,15 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) } #endif // FEATURE_ARG_SPLIT - /* If the argument tree contains an assignment (GTF_ASG) then the argument and - and every earlier argument (except constants) must be evaluated into temps - since there may be other arguments that follow and they may use the value being assigned. - - EXAMPLE: ArgTab is "a, a=5, a" - -> when we see the second arg "a=5" - we know the first two arguments "a, a=5" have to be evaluated into temps - - For the case of an assignment, we only know that there exist some assignment someplace - in the tree. We don't know what is being assigned so we are very conservative here - and assume that any local variable could have been assigned. - */ - - if (argx->gtFlags & GTF_ASG) + // If the argument tree contains an assignment (GTF_ASG) then the argument and + // and every earlier argument (except constants) must be evaluated into temps + // since there may be other arguments that follow and they may use the value being assigned. + // + // EXAMPLE: ArgTab is "a, a=5, a" + // -> when we see the second arg "a=5" + // we know the first two arguments "a, a=5" have to be evaluated into temps + // + if ((argx->gtFlags & GTF_ASG) != 0) { // If this is not the only argument, or it's a copyblk, or it // already evaluates the expression to a tmp then we need a temp in @@ -942,8 +937,8 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) assert(argx->IsValue()); } - // For all previous arguments, unless they are a simple constant - // we require that they be evaluated into temps + // For all previous arguments that may interfere with the store we + // require that they be evaluated into temps. for (CallArg& prevArg : Args()) { if (&prevArg == &arg) @@ -960,7 +955,15 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) } #endif - if ((prevArg.GetEarlyNode() != nullptr) && !prevArg.GetEarlyNode()->IsInvariant()) + if (prevArg.GetEarlyNode() == nullptr) + { + continue; + } + + bool interferes = comp->opts.OptimizationEnabled() + ? comp->gtHaveStoreInterference(argx, prevArg.GetEarlyNode()) + : !prevArg.GetEarlyNode()->IsInvariant(); + if (interferes) { SetNeedsTemp(&prevArg); } @@ -1827,6 +1830,7 @@ void CallArgs::EvalArgsToTemps(Compiler* comp, GenTreeCall* call) if (setupArg != nullptr) { arg.SetEarlyNode(setupArg); + call->gtFlags |= setupArg->gtFlags & GTF_SIDE_EFFECT; // Make sure we do not break recognition of retbuf-as-local // optimization here. If this is hit it indicates that we are @@ -3382,6 +3386,11 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call) if (makeOutArgCopy) { fgMakeOutgoingStructArgCopy(call, &arg); + + if (arg.GetEarlyNode() != nullptr) + { + flagsSummary |= arg.GetEarlyNode()->gtFlags; + } } if (argx->gtOper == GT_MKREFANY) @@ -3413,6 +3422,7 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call) // Change the expression to "(tmp=val)" arg.SetEarlyNode(store); call->gtArgs.SetTemp(&arg, tmp); + flagsSummary |= GTF_ASG; hasMultiregStructArgs |= ((arg.AbiInfo.ArgType == TYP_STRUCT) && !arg.AbiInfo.PassedByRef); #endif // !TARGET_X86 } From ad15b92e69723d4bc8607ccc526fa82e2e46ac5f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 23 Jan 2024 20:02:29 +0100 Subject: [PATCH 2/7] Nit --- src/coreclr/jit/gentree.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 6c52595a6d2b5a..54a38a77e400d3 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6338,7 +6338,7 @@ bool Compiler::gtHaveStoreInterference(GenTree* treeWithStores, GenTree* tree) class Visitor final : public GenTreeVisitor { - GenTree* m_op1; + GenTree* m_readTree; public: enum @@ -6346,7 +6346,7 @@ bool Compiler::gtHaveStoreInterference(GenTree* treeWithStores, GenTree* tree) DoPreOrder = true, }; - Visitor(Compiler* compiler, GenTree* op1) : GenTreeVisitor(compiler), m_op1(op1) + Visitor(Compiler* compiler, GenTree* readTree) : GenTreeVisitor(compiler), m_readTree(readTree) { } @@ -6360,7 +6360,7 @@ bool Compiler::gtHaveStoreInterference(GenTree* treeWithStores, GenTree* tree) if (node->OperIsLocalStore()) { - if (m_compiler->gtTreeHasLocalRead(m_op1, node->AsLclVarCommon()->GetLclNum())) + if (m_compiler->gtTreeHasLocalRead(m_readTree, node->AsLclVarCommon()->GetLclNum())) { return WALK_ABORT; } From 38ad2f07bd0b5c6dcc51445b41048b93c74d0f46 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 23 Jan 2024 22:50:06 +0100 Subject: [PATCH 3/7] Do it for MinOpts too --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/gentree.cpp | 26 ++++++++++++++++++-------- src/coreclr/jit/morph.cpp | 5 +---- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 319bc836dd589f..17ca7e633443e6 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3487,7 +3487,7 @@ class Compiler bool gtMarkAddrMode(GenTree* addr, int* costEx, int* costSz, var_types type); unsigned gtSetEvalOrder(GenTree* tree); - bool gtHaveStoreInterference(GenTree* treeWithStores, GenTree* tree); + bool gtMayHaveStoreInterference(GenTree* treeWithStores, GenTree* tree); bool gtTreeHasLocalRead(GenTree* tree, unsigned lclNum); void gtSetStmtInfo(Statement* stmt); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 54a38a77e400d3..8188ef847ead5b 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -5935,7 +5935,7 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) } // In case op2 assigns to a local var that is used in op1, we have to evaluate op1 first. - if (gtHaveStoreInterference(op2, op1)) + if (gtMayHaveStoreInterference(op2, op1)) { // TODO-ASG-Cleanup: move this guard to "gtCanSwapOrder". allowReversal = false; @@ -6318,18 +6318,20 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) #endif //------------------------------------------------------------------------ -// gtHaveStoreInterference: Check if two trees interfere because of a store in -// one of the trees. +// gtMayHaveStoreInterference: Check if two trees may interfere because of a +// store in one of the trees. // // Parameters: // treeWithStores - Tree that may have stores in it -// tree - Tree that may be reading from a store in "treeWithStores" +// tree - Tree that may be reading from a local stored to in "treeWithStores" // // Returns: -// True if there is any GT_LCL_VAR or GT_LCL_FLD node whose value depends on -// "lclNum". +// False if there is no interference. Returns true if there is any GT_LCL_VAR +// or GT_LCL_FLD node whose value depends on "lclNum". May also return true +// in cases without interference if the trees are too large and the function +// runs out of budget. // -bool Compiler::gtHaveStoreInterference(GenTree* treeWithStores, GenTree* tree) +bool Compiler::gtMayHaveStoreInterference(GenTree* treeWithStores, GenTree* tree) { if (((treeWithStores->gtFlags & GTF_ASG) == 0) || tree->IsInvariant()) { @@ -6339,6 +6341,7 @@ bool Compiler::gtHaveStoreInterference(GenTree* treeWithStores, GenTree* tree) class Visitor final : public GenTreeVisitor { GenTree* m_readTree; + unsigned m_numStoresChecked = 0; public: enum @@ -6360,10 +6363,17 @@ bool Compiler::gtHaveStoreInterference(GenTree* treeWithStores, GenTree* tree) if (node->OperIsLocalStore()) { - if (m_compiler->gtTreeHasLocalRead(m_readTree, node->AsLclVarCommon()->GetLclNum())) + // Check up to 8 stores before we bail with a conservative + // answer. Avoids quadratic behavior in case we have a large + // number of stores (e.g. created by physical promotion or by + // call args morphing). + if ((m_numStoresChecked >= 8) || + m_compiler->gtTreeHasLocalRead(m_readTree, node->AsLclVarCommon()->GetLclNum())) { return WALK_ABORT; } + + m_numStoresChecked++; } return WALK_CONTINUE; diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index cedc7628f716ca..cfe29c54e852c6 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -960,10 +960,7 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) continue; } - bool interferes = comp->opts.OptimizationEnabled() - ? comp->gtHaveStoreInterference(argx, prevArg.GetEarlyNode()) - : !prevArg.GetEarlyNode()->IsInvariant(); - if (interferes) + if (comp->gtMayHaveStoreInterference(argx, prevArg.GetEarlyNode())) { SetNeedsTemp(&prevArg); } From 9d742cd6490dc2da8c5daddb493c0e90e5f712dc Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 23 Jan 2024 22:54:16 +0100 Subject: [PATCH 4/7] Avoid unnecessary interference checks --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index cfe29c54e852c6..6623bd759946db 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -955,7 +955,7 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) } #endif - if (prevArg.GetEarlyNode() == nullptr) + if ((prevArg.GetEarlyNode() == nullptr) || prevArg.m_needTmp) { continue; } From 7da0edbdb2b734055d6cd213a9365248d8c4f395 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 23 Jan 2024 23:11:53 +0100 Subject: [PATCH 5/7] Make more obviously correct --- src/coreclr/jit/gentree.cpp | 7 ++++--- src/coreclr/jit/morph.cpp | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 8188ef847ead5b..64e643bdd6ea9a 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6327,9 +6327,10 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) // // Returns: // False if there is no interference. Returns true if there is any GT_LCL_VAR -// or GT_LCL_FLD node whose value depends on "lclNum". May also return true -// in cases without interference if the trees are too large and the function -// runs out of budget. +// or GT_LCL_FLD in "tree" whose value depends on a local stored in +// "treeWithStores". May also return true in cases without interference if +// the trees are too large and the function runs out of budget, or in +// MinOpts. // bool Compiler::gtMayHaveStoreInterference(GenTree* treeWithStores, GenTree* tree) { diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 6623bd759946db..1ce1b840de778e 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -960,7 +960,8 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) continue; } - if (comp->gtMayHaveStoreInterference(argx, prevArg.GetEarlyNode())) + if (((prevArg.GetEarlyNode()->gtFlags & GTF_ALL_EFFECT) != 0) || + comp->gtMayHaveStoreInterference(argx, prevArg.GetEarlyNode())) { SetNeedsTemp(&prevArg); } From 4b7504c41d34dcc12c9d0f95a8297142517d6c38 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 24 Jan 2024 13:13:43 +0100 Subject: [PATCH 6/7] Switch `optRedundantRelop` to use new helpers --- src/coreclr/jit/gentree.cpp | 3 +- src/coreclr/jit/redundantbranchopts.cpp | 47 ++++++++++++------------- 2 files changed, 23 insertions(+), 27 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 64e643bdd6ea9a..9993cc980a35ca 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6329,8 +6329,7 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) // False if there is no interference. Returns true if there is any GT_LCL_VAR // or GT_LCL_FLD in "tree" whose value depends on a local stored in // "treeWithStores". May also return true in cases without interference if -// the trees are too large and the function runs out of budget, or in -// MinOpts. +// the trees are too large and the function runs out of budget. // bool Compiler::gtMayHaveStoreInterference(GenTree* treeWithStores, GenTree* tree) { diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 0201a62ddf7b7f..bd2e7db92556b7 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -2038,10 +2038,18 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) continue; } + if (!prevTree->OperIs(GT_STORE_LCL_VAR)) + { + JITDUMP(" -- prev tree not STORE_LCL_VAR\n"); + break; + } + + GenTree* const prevTreeData = prevTree->AsLclVar()->Data(); + // If prevTree has side effects, bail, unless it is in the immediately preceding statement. // We'll handle exceptional side effects with VNs below. // - if ((prevTree->gtFlags & (GTF_CALL | GTF_ORDER_SIDEEFF)) != 0) + if (((prevTree->gtFlags & (GTF_CALL | GTF_ORDER_SIDEEFF)) != 0) || ((prevTreeData->gtFlags & GTF_ASG) != 0)) { if (prevStmt->GetNextStmt() != stmt) { @@ -2053,14 +2061,6 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) sideEffect = true; } - if (!prevTree->OperIs(GT_STORE_LCL_VAR)) - { - JITDUMP(" -- prev tree not STORE_LCL_VAR\n"); - break; - } - - GenTree* const prevTreeData = prevTree->AsLclVar()->Data(); - // If we are seeing PHIs we have run out of interesting stmts. // if (prevTreeData->OperIs(GT_PHI)) @@ -2069,15 +2069,6 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) break; } - // Bail if value has an embedded assignment. We could handle this - // if we generalized the interference check we run below. - // - if ((prevTreeData->gtFlags & GTF_ASG) != 0) - { - JITDUMP(" -- prev tree RHS has embedded assignment\n"); - break; - } - // Figure out what local is assigned here. // const unsigned prevTreeLclNum = prevTree->AsLclVarCommon()->GetLclNum(); @@ -2102,6 +2093,14 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) definedLocals[definedLocalsCount++] = prevTreeLclNum; + // Heuristic: only forward sub a relop + // + if (!prevTreeData->OperIsCompare()) + { + JITDUMP(" -- prev tree is not relop\n"); + continue; + } + // If the normal liberal VN of RHS is the normal liberal VN of the current tree, or is "related", // consider forward sub. // @@ -2148,7 +2147,7 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) for (unsigned int i = 0; i < definedLocalsCount; i++) { - if (gtHasRef(prevTreeData, definedLocals[i])) + if (gtTreeHasLocalRead(prevTreeData, definedLocals[i])) { JITDUMP(" -- prev tree ref to V%02u interferes\n", definedLocals[i]); interferes = true; @@ -2161,12 +2160,10 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) break; } - // Heuristic: only forward sub a relop - // - if (!prevTreeData->OperIsCompare()) + if (gtMayHaveStoreInterference(prevTreeData, tree)) { - JITDUMP(" -- prev tree is not relop\n"); - continue; + JITDUMP(" -- prev tree has an embedded store that interferes with [%06u]\n", dspTreeID(tree)); + break; } // If the lcl defined here is live out, forward sub is problematic. @@ -2191,7 +2188,7 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) // replacing. for (Statement* cur = prevStmt->GetNextStmt(); cur != stmt; cur = cur->GetNextStmt()) { - if (gtHasRef(cur->GetRootNode(), prevTreeLclNum)) + if (gtTreeHasLocalRead(cur->GetRootNode(), prevTreeLclNum)) { JITDUMP("-- prev tree has GTF_GLOB_REF and " FMT_STMT " has an interfering use\n", cur->GetID()); hasExtraUses = true; From 87b85cef45807094f35849e4d2e03481a4955620 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 24 Jan 2024 13:17:36 +0100 Subject: [PATCH 7/7] Revert a change --- src/coreclr/jit/redundantbranchopts.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index bd2e7db92556b7..38f614d8d3c8c3 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -2093,14 +2093,6 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) definedLocals[definedLocalsCount++] = prevTreeLclNum; - // Heuristic: only forward sub a relop - // - if (!prevTreeData->OperIsCompare()) - { - JITDUMP(" -- prev tree is not relop\n"); - continue; - } - // If the normal liberal VN of RHS is the normal liberal VN of the current tree, or is "related", // consider forward sub. // @@ -2166,6 +2158,14 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) break; } + // Heuristic: only forward sub a relop + // + if (!prevTreeData->OperIsCompare()) + { + JITDUMP(" -- prev tree is not relop\n"); + continue; + } + // If the lcl defined here is live out, forward sub is problematic. // We'll either create a redundant tree (as the original won't be dead) // or lose the def (if we actually move the RHS tree).