From a44cb1aa18719d6bd109d7e29b175bb1564744c3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 22 Jul 2024 16:50:44 +0200 Subject: [PATCH 01/10] Slightly improve MinOpts JIT TP --- src/coreclr/jit/compiler.h | 18 ++++++++++++++++-- src/coreclr/jit/gentree.cpp | 12 +++--------- src/coreclr/jit/importercalls.cpp | 8 ++------ src/coreclr/jit/morph.cpp | 5 ++++- src/coreclr/jit/vartype.h | 2 +- 5 files changed, 26 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index ce8cfe9f57f0d..cca222e0c3e1a 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -9985,6 +9985,8 @@ class Compiler // Maximum number of locals before turning off the inlining #define MAX_LV_NUM_COUNT_FOR_INLINING 512 + bool canUseTier0Opts; + bool canUseAllOpts; bool compMinOpts; bool compMinOptsIsSet; #ifdef DEBUG @@ -10011,13 +10013,22 @@ class Compiler } #endif // !DEBUG + // TODO: we should convert these into a single OptimizationLevel + bool OptimizationDisabled() const { - return MinOpts() || compDbgCode; + assert(compMinOptsIsSet); + return !canUseAllOpts; } bool OptimizationEnabled() const { - return !OptimizationDisabled(); + assert(compMinOptsIsSet); + return canUseAllOpts; + } + bool Tier0OptimizationEnabled() const + { + assert(compMinOptsIsSet); + return canUseTier0Opts; } void SetMinOpts(bool val) @@ -10026,6 +10037,9 @@ class Compiler assert(!compMinOptsIsSet || (compMinOpts == val)); compMinOpts = val; compMinOptsIsSet = true; + + canUseTier0Opts = !compDbgCode && !jitFlags->IsSet(JitFlags::JIT_FLAG_MIN_OPT); + canUseAllOpts = canUseTier0Opts && !val; } // true if the CLFLG_* for an optimization is set. diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 8ba61fe3a1121..a6ed22bda72cb 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -13340,10 +13340,7 @@ GenTree* Compiler::gtFoldExpr(GenTree* tree) return tree; } - // NOTE: MinOpts() is always true for Tier0 so we have to check explicit flags instead. - // To be fixed in https://github.com/dotnet/runtime/pull/77465 - const bool tier0opts = !opts.compDbgCode && !opts.jitFlags->IsSet(JitFlags::JIT_FLAG_MIN_OPT); - if (!tier0opts) + if (!opts.Tier0OptimizationEnabled()) { return tree; } @@ -13406,7 +13403,7 @@ GenTree* Compiler::gtFoldExpr(GenTree* tree) // special operator that can use only one constant // to fold - e.g. booleans - if (tier0opts && opts.OptimizationDisabled()) + if (opts.OptimizationDisabled()) { // Too heavy for tier0 return tree; @@ -15197,10 +15194,7 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) GenTree* op1 = tree->gtGetOp1(); GenTree* op2 = tree->gtGetOp2IfPresent(); - // NOTE: MinOpts() is always true for Tier0 so we have to check explicit flags instead. - // To be fixed in https://github.com/dotnet/runtime/pull/77465 - const bool tier0opts = !opts.compDbgCode && !opts.jitFlags->IsSet(JitFlags::JIT_FLAG_MIN_OPT); - if (!tier0opts) + if (!opts.Tier0OptimizationEnabled()) { return tree; } diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index e8de998ebed2c..566fdceb8fdf8 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -1553,7 +1553,7 @@ GenTree* Compiler::impThrowIfNull(GenTreeCall* call) assert(call->gtArgs.CountUserArgs() == 2); assert(call->TypeIs(TYP_VOID)); - if (opts.compDbgCode || opts.jitFlags->IsSet(JitFlags::JIT_FLAG_MIN_OPT)) + if (!opts.Tier0OptimizationEnabled()) { // Don't fold it for debug code or forced MinOpts return call; @@ -3302,11 +3302,7 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE clsHnd, // Allow some lighweight intrinsics in Tier0 which can improve throughput // we're fine if intrinsic decides to not expand itself in this case unlike mustExpand. - // NOTE: MinOpts() is always true for Tier0 so we have to check explicit flags instead. - // To be fixed in https://github.com/dotnet/runtime/pull/77465 - const bool tier0opts = !opts.compDbgCode && !opts.jitFlags->IsSet(JitFlags::JIT_FLAG_MIN_OPT); - - if (!mustExpand && tier0opts) + if (!mustExpand && opts.Tier0OptimizationEnabled()) { switch (ni) { diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 0d922459acdc4..23c189cbca45b 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -8328,7 +8328,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA } // Try to fold it, maybe we get lucky, - tree = gtFoldExpr(tree); + if (opts.OptimizationEnabled()) + { + tree = gtFoldExpr(tree); + } if (oldTree != tree) { diff --git a/src/coreclr/jit/vartype.h b/src/coreclr/jit/vartype.h index 34668fd545b71..c0cfa87775dab 100644 --- a/src/coreclr/jit/vartype.h +++ b/src/coreclr/jit/vartype.h @@ -185,7 +185,7 @@ inline bool varTypeIsArithmetic(T vt) template inline bool varTypeIsGC(T vt) { - return ((varTypeClassification[TypeGet(vt)] & (VTF_GCR | VTF_BYR)) != 0); + return (TypeGet(vt) == TYP_REF) || (TypeGet(vt) == TYP_BYREF); } template From 62a78e85625c62c99c7963461147a20f97ec5ebb Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 22 Jul 2024 19:31:03 +0200 Subject: [PATCH 02/10] Update morph.cpp --- src/coreclr/jit/morph.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 23c189cbca45b..0d922459acdc4 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -8328,10 +8328,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA } // Try to fold it, maybe we get lucky, - if (opts.OptimizationEnabled()) - { - tree = gtFoldExpr(tree); - } + tree = gtFoldExpr(tree); if (oldTree != tree) { From f76354d20d93ba6d32053648b835c8d63dd24446 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 23 Jul 2024 02:20:38 +0200 Subject: [PATCH 03/10] Introduce gtSetEvalOrderMinOpts --- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/gentree.cpp | 165 ++++++++++++++++++++++++++++++++++++ src/coreclr/jit/morph.cpp | 8 +- 3 files changed, 173 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index cca222e0c3e1a..ee1dcdd0b9a20 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3655,6 +3655,7 @@ class Compiler bool gtMarkAddrMode(GenTree* addr, int* costEx, int* costSz, var_types type); unsigned gtSetEvalOrder(GenTree* tree); + unsigned gtSetEvalOrderMinOpts(GenTree* tree); bool gtMayHaveStoreInterference(GenTree* treeWithStores, GenTree* tree); bool gtTreeHasLocalRead(GenTree* tree, unsigned lclNum); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index a6ed22bda72cb..8e887c28d39b8 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -4848,6 +4848,11 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) { assert(tree); + if (opts.OptimizationDisabled()) + { + return gtSetEvalOrderMinOpts(tree); + } + #ifdef DEBUG /* Clear the GTF_DEBUG_NODE_MORPHED flag as well */ tree->gtDebugFlags &= ~GTF_DEBUG_NODE_MORPHED; @@ -6212,6 +6217,166 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) #pragma warning(pop) #endif +//------------------------------------------------------------------------ +// gtSetEvalOrderMinOpts: A MinOpts specific version of gtSetEvalOrder. We don't +// need to set costs, but we're looking for opportunities to swap operands. +// +// Arguments: +// tree - The tree for which we are setting the evaluation order. +// +// Return Value: +// the Sethi 'complexity' estimate for this tree (the higher +// the number, the higher is the tree's resources requirement) +// +unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) +{ + assert(tree); + + if (tree->OperIsLeaf()) + { + // Nothing to do for leaves. + return 0; + } + + unsigned level = 1; + if (tree->OperIsSimple()) + { + GenTree* op1 = tree->AsOp()->gtOp1; + GenTree* op2 = tree->gtGetOp2IfPresent(); + + // Check for addressing mode with a null base + if (tree->OperIsAddrMode() && (op1 == nullptr)) + { + std::swap(op1, op2); + } + + // Check for a nilary operator + if (op1 == nullptr) + { + assert(op2 == nullptr); + return level; + } + + if (op2 == nullptr) + { + gtSetEvalOrderMinOpts(op1); + return level; + } + + // Default Binary ops have a cost of 1,1 + level = gtSetEvalOrderMinOpts(op1); + unsigned lvl2 = gtSetEvalOrderMinOpts(op2); + + bool allowReversal = true; + // for certain operators. + switch (tree->OperGet()) + { + case GT_COMMA: + case GT_BOUNDS_CHECK: + case GT_INTRINSIC: + case GT_QMARK: + case GT_COLON: + // We're not going to swap operands in these + allowReversal = false; + break; + + case GT_STORE_BLK: + case GT_STOREIND: + { + if (op1->IsInvariant()) + { + allowReversal = false; + tree->SetReverseOp(); + break; + } + if ((op1->gtFlags & GTF_ALL_EFFECT) != 0) + { + break; + } + + // In case op2 assigns to a local var that is used in op1, we have to evaluate op1 first. + if (gtMayHaveStoreInterference(op2, op1)) + { + // TODO-ASG-Cleanup: move this guard to "gtCanSwapOrder". + allowReversal = false; + break; + } + + // If op2 is simple then evaluate op1 first + if (op2->OperIsLeaf()) + { + break; + } + + allowReversal = false; + tree->SetReverseOp(); + break; + } + + default: + break; + } + + bool tryToSwap = false; + if (fgOrder != FGOrderLinear) + { + tryToSwap = tree->IsReverseOp() ? level > lvl2 : level < lvl2; + } + + if (tryToSwap && allowReversal) + { + // Can we swap the order by commuting the operands? + bool canSwap = tree->IsReverseOp() ? gtCanSwapOrder(op2, op1) : gtCanSwapOrder(op1, op2); + if (canSwap) + { + if (tree->OperIsCmpCompare()) + { + genTreeOps oper = tree->OperGet(); + if (GenTree::SwapRelop(oper) != oper) + { + tree->SetOper(GenTree::SwapRelop(oper)); + } + std::swap(tree->AsOp()->gtOp1, tree->AsOp()->gtOp2); + } + else if (tree->OperIsCommutative()) + { + std::swap(tree->AsOp()->gtOp1, tree->AsOp()->gtOp2); + } + else + { + // Mark the operand's evaluation order to be swapped. + tree->gtFlags ^= GTF_REVERSE_OPS; + } + } + } + + // Swap the level counts + if (tree->IsReverseOp()) + { + std::swap(level, lvl2); + } + + // Compute the sethi number for this binary operator + if (level < 1) + { + level = lvl2; + } + else if (level == lvl2) + { + level++; + } + } + else if (tree->IsCall()) + { + for (CallArg& arg : tree->AsCall()->gtArgs.EarlyArgs()) + { + gtSetEvalOrderMinOpts(arg.GetEarlyNode()); + } + level = 3; + } + return level; +} + //------------------------------------------------------------------------ // gtMayHaveStoreInterference: Check if two trees may interfere because of a // store in one of the trees. diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 0d922459acdc4..f2c06db22026b 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -1476,7 +1476,7 @@ void CallArgs::SortArgs(Compiler* comp, GenTreeCall* call, CallArg** sortedArgs) assert(begTab == endTab); break; } - else + else if (comp->opts.OptimizationEnabled()) { if (!costsPrepared) { @@ -1492,6 +1492,12 @@ void CallArgs::SortArgs(Compiler* comp, GenTreeCall* call, CallArg** sortedArgs) expensiveArg = arg; } } + else + { + // We don't have cost information in MinOpts + expensiveArgIndex = curInx; + expensiveArg = arg; + } } } From 28e67dd4234b9bb30c6998076011fed4faaa81e5 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 23 Jul 2024 02:54:11 +0200 Subject: [PATCH 04/10] fix ci --- src/coreclr/jit/compiler.cpp | 2 +- src/coreclr/jit/gentree.cpp | 2 ++ src/coreclr/jit/morph.cpp | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index e11d3cd30bbc1..b091a2efbbec5 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5273,7 +5273,7 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl #ifdef DEBUG // Stash the current estimate of the function's size if necessary. - if (verbose) + if (verbose && opts.OptimizationEnabled()) { compSizeEstimate = 0; compCycleEstimate = 0; diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 8e887c28d39b8..561af987db147 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6368,6 +6368,8 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) } else if (tree->IsCall()) { + // We ignore late args - they don't bring any noticeable benefits + // according to asmdiffs/tpdiff for (CallArg& arg : tree->AsCall()->gtArgs.EarlyArgs()) { gtSetEvalOrderMinOpts(arg.GetEarlyNode()); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index f2c06db22026b..83b55e804f56d 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -1103,7 +1103,7 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) // TODO-CQ: handle HWI/SIMD/COMMA nodes in multi-reg morphing. SetNeedsTemp(&arg); } - else + else if (opts.OptimizationEnabled()) { // Finally, we call gtPrepareCost to measure the cost of evaluating this tree. comp->gtPrepareCost(argx); From 7ffac163d2c5822bdc6b487c58a4290d4b689280 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 23 Jul 2024 03:36:28 +0200 Subject: [PATCH 05/10] Update morph.cpp --- 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 83b55e804f56d..0f0753598f786 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -1103,7 +1103,7 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) // TODO-CQ: handle HWI/SIMD/COMMA nodes in multi-reg morphing. SetNeedsTemp(&arg); } - else if (opts.OptimizationEnabled()) + else if (comp->opts.OptimizationEnabled()) { // Finally, we call gtPrepareCost to measure the cost of evaluating this tree. comp->gtPrepareCost(argx); From afb150f5a541916317c0249b353a21b9cfac3aed Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 23 Jul 2024 14:15:39 +0200 Subject: [PATCH 06/10] Clean up --- src/coreclr/jit/gentree.cpp | 54 ++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 561af987db147..bea1e39117336 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6231,10 +6231,15 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) { assert(tree); + if (fgOrder == FGOrderLinear) + { + // We don't re-order operands in LIR anyway. + return 0; + } if (tree->OperIsLeaf()) { - // Nothing to do for leaves. + // Nothing to do for leaves, report as having Sethi 'complexity' of 0 return 0; } @@ -6244,8 +6249,8 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) GenTree* op1 = tree->AsOp()->gtOp1; GenTree* op2 = tree->gtGetOp2IfPresent(); - // Check for addressing mode with a null base - if (tree->OperIsAddrMode() && (op1 == nullptr)) + // Only GT_LEA may have a nullptr op1 and a non-nullptr op2 + if (tree->OperIs(GT_LEA) && (op1 == nullptr)) { std::swap(op1, op2); } @@ -6263,12 +6268,11 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) return level; } - // Default Binary ops have a cost of 1,1 - level = gtSetEvalOrderMinOpts(op1); - unsigned lvl2 = gtSetEvalOrderMinOpts(op2); + level = gtSetEvalOrderMinOpts(op1); + unsigned levelOp2 = gtSetEvalOrderMinOpts(op2); - bool allowReversal = true; - // for certain operators. + bool allowSwap = true; + // TODO: Introduce a function to check whether we can swap the order of its operands or not. switch (tree->OperGet()) { case GT_COMMA: @@ -6277,7 +6281,7 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) case GT_QMARK: case GT_COLON: // We're not going to swap operands in these - allowReversal = false; + allowSwap = false; break; case GT_STORE_BLK: @@ -6285,7 +6289,7 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) { if (op1->IsInvariant()) { - allowReversal = false; + allowSwap = false; tree->SetReverseOp(); break; } @@ -6298,7 +6302,7 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) if (gtMayHaveStoreInterference(op2, op1)) { // TODO-ASG-Cleanup: move this guard to "gtCanSwapOrder". - allowReversal = false; + allowSwap = false; break; } @@ -6308,7 +6312,7 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) break; } - allowReversal = false; + allowSwap = false; tree->SetReverseOp(); break; } @@ -6317,16 +6321,11 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) break; } - bool tryToSwap = false; - if (fgOrder != FGOrderLinear) - { - tryToSwap = tree->IsReverseOp() ? level > lvl2 : level < lvl2; - } - - if (tryToSwap && allowReversal) + const bool shouldSwap = tree->IsReverseOp() ? level > levelOp2 : level < levelOp2; + if (shouldSwap && allowSwap) { // Can we swap the order by commuting the operands? - bool canSwap = tree->IsReverseOp() ? gtCanSwapOrder(op2, op1) : gtCanSwapOrder(op1, op2); + const bool canSwap = tree->IsReverseOp() ? gtCanSwapOrder(op2, op1) : gtCanSwapOrder(op1, op2); if (canSwap) { if (tree->OperIsCmpCompare()) @@ -6353,15 +6352,15 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) // Swap the level counts if (tree->IsReverseOp()) { - std::swap(level, lvl2); + std::swap(level, levelOp2); } // Compute the sethi number for this binary operator if (level < 1) { - level = lvl2; + level = levelOp2; } - else if (level == lvl2) + else if (level == levelOp2) { level++; } @@ -6376,6 +6375,10 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) } level = 3; } + + // NOTE: we skip many operators here (e.g. GT_HWINTRINSIC) + // in order to maintain a good trade-off between CQ and TP. + return level; } @@ -30428,10 +30431,7 @@ GenTree* Compiler::gtFoldExprHWIntrinsic(GenTreeHWIntrinsic* tree) { assert(tree->OperIsHWIntrinsic()); - // NOTE: MinOpts() is always true for Tier0 so we have to check explicit flags instead. - // To be fixed in https://github.com/dotnet/runtime/pull/77465 - const bool tier0opts = !opts.compDbgCode && !opts.jitFlags->IsSet(JitFlags::JIT_FLAG_MIN_OPT); - if (!tier0opts) + if (!opts.Tier0OptimizationEnabled()) { return tree; } From 7a2a7ace955f0db715031b23677957e0e83d5166 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 23 Jul 2024 18:34:24 +0200 Subject: [PATCH 07/10] test --- src/coreclr/jit/gentree.cpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index bea1e39117336..176a01778dcdd 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6375,9 +6375,18 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) } level = 3; } +#if defined(FEATURE_HW_INTRINSICS) + else if (tree->OperIsHWIntrinsic()) + { + for (size_t i = tree->AsMultiOp()->GetOperandCount(); i >= 1; i--) + { + unsigned lvl = gtSetEvalOrderMinOpts(tree->AsMultiOp()->Op(i)); + level = max(lvl, level + 1); + } + } +#endif // FEATURE_HW_INTRINSICS - // NOTE: we skip many operators here (e.g. GT_HWINTRINSIC) - // in order to maintain a good trade-off between CQ and TP. + // NOTE: we skip many operators here in order to maintain a good trade-off between CQ and TP. return level; } From 029a7479c899ba59c13b280a742757d903bcbbaf Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 23 Jul 2024 18:59:38 +0200 Subject: [PATCH 08/10] Clean up --- src/coreclr/jit/gentree.cpp | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 176a01778dcdd..f36633fb01efc 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -3931,8 +3931,10 @@ unsigned Compiler::gtSetMultiOpOrder(GenTreeMultiOp* multiOp) int costSz = 1; unsigned level = 0; + bool optsEnabled = opts.OptimizationEnabled(); + #if defined(FEATURE_HW_INTRINSICS) - if (multiOp->OperIs(GT_HWINTRINSIC)) + if (multiOp->OperIs(GT_HWINTRINSIC) && optsEnabled) { GenTreeHWIntrinsic* hwTree = multiOp->AsHWIntrinsic(); #if defined(TARGET_XARCH) @@ -4052,8 +4054,12 @@ unsigned Compiler::gtSetMultiOpOrder(GenTreeMultiOp* multiOp) level += 1; } - costEx += (multiOp->Op(1)->GetCostEx() + multiOp->Op(2)->GetCostEx()); - costSz += (multiOp->Op(1)->GetCostSz() + multiOp->Op(2)->GetCostSz()); + if (optsEnabled) + { + // We don't need/have costs in MinOpts + costEx += (multiOp->Op(1)->GetCostEx() + multiOp->Op(2)->GetCostEx()); + costSz += (multiOp->Op(1)->GetCostSz() + multiOp->Op(2)->GetCostSz()); + } } else { @@ -4064,12 +4070,19 @@ unsigned Compiler::gtSetMultiOpOrder(GenTreeMultiOp* multiOp) level = max(lvl, level + 1); - costEx += op->GetCostEx(); - costSz += op->GetCostSz(); + if (optsEnabled) + { + // We don't need/have costs in MinOpts + costEx += op->GetCostEx(); + costSz += op->GetCostSz(); + } } } - multiOp->SetCosts(costEx, costSz); + if (optsEnabled) + { + multiOp->SetCosts(costEx, costSz); + } return level; } #endif @@ -6378,11 +6391,7 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) #if defined(FEATURE_HW_INTRINSICS) else if (tree->OperIsHWIntrinsic()) { - for (size_t i = tree->AsMultiOp()->GetOperandCount(); i >= 1; i--) - { - unsigned lvl = gtSetEvalOrderMinOpts(tree->AsMultiOp()->Op(i)); - level = max(lvl, level + 1); - } + return gtSetMultiOpOrder(tree->AsMultiOp()); } #endif // FEATURE_HW_INTRINSICS From dc38df23e30e6ccbe71628bc80b93a5fa456edf3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 23 Jul 2024 23:02:52 +0200 Subject: [PATCH 09/10] clean up --- src/coreclr/jit/gentree.cpp | 99 +++++++++++++++---------------------- 1 file changed, 41 insertions(+), 58 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index f36633fb01efc..0cc5e18351fb1 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -4836,6 +4836,42 @@ bool Compiler::gtMarkAddrMode(GenTree* addr, int* pCostEx, int* pCostSz, var_typ return false; } +static void SetIndirectStoreEvalOrder(Compiler* comp, GenTreeStoreInd* store, bool* allowReversal) +{ + GenTree* addr = store->Addr(); + GenTree* data = store->Data(); + *allowReversal = true; + + if (addr->IsInvariant()) + { + *allowReversal = false; + store->SetReverseOp(); + return; + } + + if ((addr->gtFlags & GTF_ALL_EFFECT) != 0) + { + return; + } + + // In case op2 assigns to a local var that is used in op1, we have to evaluate op1 first. + if (comp->gtMayHaveStoreInterference(data, addr)) + { + // TODO-ASG-Cleanup: move this guard to "gtCanSwapOrder". + *allowReversal = false; + return; + } + + // If op2 is simple then evaluate op1 first + if (data->OperIsLeaf()) + { + return; + } + + *allowReversal = false; + store->SetReverseOp(); +} + /***************************************************************************** * * Given a tree, figure out the order in which its sub-operands should be @@ -5856,33 +5892,7 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) // TODO-ASG-Cleanup: this logic emulated the ASG case below. See how of much of it can be deleted. if (!optValnumCSE_phase || optCSE_canSwap(op1, op2)) { - if (op1->IsInvariant()) - { - allowReversal = false; - tree->SetReverseOp(); - break; - } - if ((op1->gtFlags & GTF_ALL_EFFECT) != 0) - { - break; - } - - // In case op2 assigns to a local var that is used in op1, we have to evaluate op1 first. - if (gtMayHaveStoreInterference(op2, op1)) - { - // TODO-ASG-Cleanup: move this guard to "gtCanSwapOrder". - allowReversal = false; - break; - } - - // If op2 is simple then evaluate op1 first - if (op2->OperIsLeaf()) - { - break; - } - - allowReversal = false; - tree->SetReverseOp(); + SetIndirectStoreEvalOrder(this, tree->AsStoreInd(), &allowReversal); } break; @@ -6271,14 +6281,15 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) // Check for a nilary operator if (op1 == nullptr) { + // E.g. void GT_RETURN, GT_RETFIT assert(op2 == nullptr); - return level; + return 0; } if (op2 == nullptr) { gtSetEvalOrderMinOpts(op1); - return level; + return 1; } level = gtSetEvalOrderMinOpts(op1); @@ -6299,36 +6310,8 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) case GT_STORE_BLK: case GT_STOREIND: - { - if (op1->IsInvariant()) - { - allowSwap = false; - tree->SetReverseOp(); - break; - } - if ((op1->gtFlags & GTF_ALL_EFFECT) != 0) - { - break; - } - - // In case op2 assigns to a local var that is used in op1, we have to evaluate op1 first. - if (gtMayHaveStoreInterference(op2, op1)) - { - // TODO-ASG-Cleanup: move this guard to "gtCanSwapOrder". - allowSwap = false; - break; - } - - // If op2 is simple then evaluate op1 first - if (op2->OperIsLeaf()) - { - break; - } - - allowSwap = false; - tree->SetReverseOp(); + SetIndirectStoreEvalOrder(this, tree->AsStoreInd(), &allowSwap); break; - } default: break; From 93dcf5912dc82dfed5460d23e7c290f3afd651a6 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 24 Jul 2024 00:11:41 +0200 Subject: [PATCH 10/10] fix build --- src/coreclr/jit/gentree.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 0cc5e18351fb1..88d087797a259 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -4836,8 +4836,10 @@ bool Compiler::gtMarkAddrMode(GenTree* addr, int* pCostEx, int* pCostSz, var_typ return false; } -static void SetIndirectStoreEvalOrder(Compiler* comp, GenTreeStoreInd* store, bool* allowReversal) +static void SetIndirectStoreEvalOrder(Compiler* comp, GenTreeIndir* store, bool* allowReversal) { + assert(store->OperIs(GT_STORE_BLK, GT_STOREIND)); + GenTree* addr = store->Addr(); GenTree* data = store->Data(); *allowReversal = true; @@ -5892,7 +5894,7 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) // TODO-ASG-Cleanup: this logic emulated the ASG case below. See how of much of it can be deleted. if (!optValnumCSE_phase || optCSE_canSwap(op1, op2)) { - SetIndirectStoreEvalOrder(this, tree->AsStoreInd(), &allowReversal); + SetIndirectStoreEvalOrder(this, tree->AsIndir(), &allowReversal); } break; @@ -6310,7 +6312,7 @@ unsigned Compiler::gtSetEvalOrderMinOpts(GenTree* tree) case GT_STORE_BLK: case GT_STOREIND: - SetIndirectStoreEvalOrder(this, tree->AsStoreInd(), &allowSwap); + SetIndirectStoreEvalOrder(this, tree->AsIndir(), &allowSwap); break; default: