Slightly improve MinOpts JIT TP #105250
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) && optsEnabled) | ||
| { | ||
| GenTreeHWIntrinsic* hwTree = multiOp->AsHWIntrinsic(); | ||
| #if defined(TARGET_XARCH) | ||
|
|
@@ -4052,8 +4054,12 @@ unsigned Compiler::gtSetMultiOpOrder(GenTreeMultiOp* multiOp) | |
| level += 1; | ||
| } | ||
|
|
||
| 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); | ||
|
|
||
| if (optsEnabled) | ||
| { | ||
| // We don't need/have costs in MinOpts | ||
| costEx += op->GetCostEx(); | ||
| costSz += op->GetCostSz(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (optsEnabled) | ||
| { | ||
| multiOp->SetCosts(costEx, costSz); | ||
| } | ||
| return level; | ||
| } | ||
| #endif | ||
|
|
@@ -4848,6 +4861,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 +6230,176 @@ 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 (fgOrder == FGOrderLinear) | ||
| { | ||
| // We don't re-order operands in LIR anyway. | ||
| return 0; | ||
|
Comment on lines
+6261
to
+6262
There was a problem hiding this comment.
Not sure, but the original function |
||
| } | ||
|
|
||
| if (tree->OperIsLeaf()) | ||
| { | ||
| // Nothing to do for leaves, report as having Sethi 'complexity' of 0 | ||
| return 0; | ||
| } | ||
|
|
||
| unsigned level = 1; | ||
| if (tree->OperIsSimple()) | ||
|
There was a problem hiding this comment. nit: Might be worth a small comment saying that we are intentionally not handling hwintrinsics (they otherwise normally support GTF_REVERSE_OPS and other considerations). There was a problem hiding this comment. I'll add a comment, but we skip a lot of trees here, it was driven by asmdiff + tpdiff results There was a problem hiding this comment.
👍, had mostly thought it might be worth a callout because it normally mirrors a lot of the |
||
| { | ||
| GenTree* op1 = tree->AsOp()->gtOp1; | ||
| GenTree* op2 = tree->gtGetOp2IfPresent(); | ||
|
|
||
| // Only GT_LEA may have a nullptr op1 and a non-nullptr op2 | ||
| if (tree->OperIs(GT_LEA) && (op1 == nullptr)) | ||
| { | ||
| std::swap(op1, op2); | ||
| } | ||
|
|
||
| // Check for a nilary operator | ||
| if (op1 == nullptr) | ||
| { | ||
| assert(op2 == nullptr); | ||
|
There was a problem hiding this comment. In what cases do we get here? There was a problem hiding this comment. it's also copied from the original function. Looks like |
||
| return level; | ||
| } | ||
|
|
||
| if (op2 == nullptr) | ||
| { | ||
| gtSetEvalOrderMinOpts(op1); | ||
| return level; | ||
| } | ||
|
|
||
| level = gtSetEvalOrderMinOpts(op1); | ||
| unsigned levelOp2 = gtSetEvalOrderMinOpts(op2); | ||
|
|
||
| bool allowSwap = true; | ||
| // TODO: Introduce a function to check whether we can swap the order of its operands or not. | ||
|
There was a problem hiding this comment. Should this TODO be removed? Not sure I understand what it refers to. If it refers to extract the below logic into a function, why not just do that in this PR? There was a problem hiding this comment. I'd better remove the comment then for now, I don't mind. The idea is to share quirks with the Tier1 version (esp for STOREIND/STORE_BLK) There was a problem hiding this comment. Might be useful enough to leave a comment that it should be kept in sync with the quirks in the tier 1 version (and likewise in the tier 1 version) There was a problem hiding this comment. Moved both to a single helper, to keep the quirks in one place |
||
| 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 | ||
| allowSwap = false; | ||
| break; | ||
|
|
||
| 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)) | ||
|
There was a problem hiding this comment. You probably don't need this expensive version in MinOpts. It can be the conservative There was a problem hiding this comment.
just tried - it adds +28k bytes size regression for There was a problem hiding this comment. Ah ok, fine to leave it then. (Actually looks like I called out explicitly doing it in MinOpts in that PR as well) |
||
| { | ||
| // 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(); | ||
| break; | ||
| } | ||
|
|
||
| default: | ||
| break; | ||
| } | ||
|
|
||
| const bool shouldSwap = tree->IsReverseOp() ? level > levelOp2 : level < levelOp2; | ||
| if (shouldSwap && allowSwap) | ||
| { | ||
| // Can we swap the order by commuting the operands? | ||
| const 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, levelOp2); | ||
| } | ||
|
|
||
| // Compute the sethi number for this binary operator | ||
| if (level < 1) | ||
| { | ||
| level = levelOp2; | ||
| } | ||
| else if (level == levelOp2) | ||
| { | ||
| level++; | ||
| } | ||
| } | ||
| 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()); | ||
| } | ||
| level = 3; | ||
| } | ||
| #if defined(FEATURE_HW_INTRINSICS) | ||
| else if (tree->OperIsHWIntrinsic()) | ||
| { | ||
| return gtSetMultiOpOrder(tree->AsMultiOp()); | ||
| } | ||
| #endif // FEATURE_HW_INTRINSICS | ||
|
|
||
| // NOTE: we skip many operators here in order to maintain a good trade-off between CQ and TP. | ||
|
|
||
| return level; | ||
| } | ||
|
|
||
| //------------------------------------------------------------------------ | ||
| // gtMayHaveStoreInterference: Check if two trees may interfere because of a | ||
| // store in one of the trees. | ||
|
|
@@ -13340,10 +13528,7 @@ GenTree* Compiler::gtFoldExpr(GenTree* tree) | |
| return tree; | ||
| } | ||
|
|
||
| if (!opts.Tier0OptimizationEnabled()) | ||
tannergooding marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| return tree; | ||
| } | ||
|
|
@@ -13406,7 +13591,7 @@ GenTree* Compiler::gtFoldExpr(GenTree* tree) | |
| // special operator that can use only one constant | ||
| // to fold - e.g. booleans | ||
|
|
||
| if (opts.OptimizationDisabled()) | ||
| { | ||
| // Too heavy for tier0 | ||
| return tree; | ||
|
|
@@ -15197,10 +15382,7 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) | |
| GenTree* op1 = tree->gtGetOp1(); | ||
| GenTree* op2 = tree->gtGetOp2IfPresent(); | ||
|
|
||
| if (!opts.Tier0OptimizationEnabled()) | ||
| { | ||
| return tree; | ||
| } | ||
|
|
@@ -30267,10 +30449,7 @@ GenTree* Compiler::gtFoldExprHWIntrinsic(GenTreeHWIntrinsic* tree) | |
| { | ||
| assert(tree->OperIsHWIntrinsic()); | ||
|
|
||
| if (!opts.Tier0OptimizationEnabled()) | ||
| { | ||
| return tree; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor nit is that
OptimizationEnabledandTier0OptimizationEnabledboth existing may be a bit confusing. Is there any idea what we would call the levels if this were some kind ofOptimizationLevelenum, just for ideas on what we could call it as an alternative?I think this is fine for now, because getting better names is a more involved change; just wanted to check if there were any other ideas/suggestions for the interim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I agree that it doesn't look pretty, but a proper refactoring will be quite big and hard to review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet. Andy suggested to use OptLevel: NoOpts, Size, Speed where Size implies "light-weight". The problem that we already have optLevel in jit, but we never use it (it's always Blended) so it all needs some re-thinking