Slightly improve MinOpts JIT TP #105250
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,168 @@ 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()) | ||
|
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(); | ||
|
|
||
| // 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); | ||
|
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; | ||
| } | ||
|
|
||
| // 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)) | ||
|
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". | ||
| 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()) | ||
| { | ||
| // 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; | ||
| } | ||
| return level; | ||
| } | ||
|
|
||
| //------------------------------------------------------------------------ | ||
| // gtMayHaveStoreInterference: Check if two trees may interfere because of a | ||
| // store in one of the trees. | ||
|
|
@@ -13340,10 +13507,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 +13570,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 +15361,7 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) | |
| GenTree* op1 = tree->gtGetOp1(); | ||
| GenTree* op2 = tree->gtGetOp2IfPresent(); | ||
|
|
||
| if (!opts.Tier0OptimizationEnabled()) | ||
| { | ||
| return tree; | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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