diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index d5b465d6dfa82..17ca7e633443e 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 gtMayHaveStoreInterference(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 f1f1afd357bce..86f7f3ecee8de 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 ac88ed8abae8b..9993cc980a35c 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 (gtMayHaveStoreInterference(op2, op1)) { // TODO-ASG-Cleanup: move this guard to "gtCanSwapOrder". allowReversal = false; @@ -6317,6 +6317,135 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) #pragma warning(pop) #endif +//------------------------------------------------------------------------ +// 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 local stored to in "treeWithStores" +// +// Returns: +// 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. +// +bool Compiler::gtMayHaveStoreInterference(GenTree* treeWithStores, GenTree* tree) +{ + if (((treeWithStores->gtFlags & GTF_ASG) == 0) || tree->IsInvariant()) + { + return false; + } + + class Visitor final : public GenTreeVisitor + { + GenTree* m_readTree; + unsigned m_numStoresChecked = 0; + + public: + enum + { + DoPreOrder = true, + }; + + Visitor(Compiler* compiler, GenTree* readTree) : GenTreeVisitor(compiler), m_readTree(readTree) + { + } + + Compiler::fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) + { + GenTree* node = *use; + if ((node->gtFlags & GTF_ASG) == 0) + { + return WALK_SKIP_SUBTREES; + } + + if (node->OperIsLocalStore()) + { + // 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; + } + }; + + 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 33428d5fd5a11..1ce1b840de778 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,13 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) } #endif - if ((prevArg.GetEarlyNode() != nullptr) && !prevArg.GetEarlyNode()->IsInvariant()) + if ((prevArg.GetEarlyNode() == nullptr) || prevArg.m_needTmp) + { + continue; + } + + if (((prevArg.GetEarlyNode()->gtFlags & GTF_ALL_EFFECT) != 0) || + comp->gtMayHaveStoreInterference(argx, prevArg.GetEarlyNode())) { SetNeedsTemp(&prevArg); } @@ -1827,6 +1828,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 +3384,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 +3420,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 } diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 0201a62ddf7b7..38f614d8d3c8c 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(); @@ -2148,7 +2139,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,6 +2152,12 @@ bool Compiler::optRedundantRelop(BasicBlock* const block) break; } + if (gtMayHaveStoreInterference(prevTreeData, tree)) + { + JITDUMP(" -- prev tree has an embedded store that interferes with [%06u]\n", dspTreeID(tree)); + break; + } + // Heuristic: only forward sub a relop // if (!prevTreeData->OperIsCompare()) @@ -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;