JIT: limited version of forward substitution for some relops #61023
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ PhaseStatus Compiler::optRedundantBranches() | |
| // | ||
| if (block->bbJumpKind == BBJ_COND) | ||
| { | ||
| madeChanges |= m_compiler->optRedundantRelop(block); | ||
| madeChanges |= m_compiler->optRedundantBranch(block); | ||
| } | ||
| } | ||
|
|
@@ -98,7 +99,7 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) | |
|
|
||
| GenTree* const tree = jumpTree->AsOp()->gtOp1; | ||
|
|
||
| if (!tree->OperIsCompare()) | ||
| { | ||
| return false; | ||
| } | ||
|
|
@@ -691,6 +692,294 @@ bool Compiler::optJumpThread(BasicBlock* const block, BasicBlock* const domBlock | |
| return true; | ||
| } | ||
|
|
||
| //------------------------------------------------------------------------ | ||
| // optRedundantRelop: see if the value of tree is redundant given earlier | ||
| // relops in this block. | ||
| // | ||
| // Arguments: | ||
| // block - block of interest (BBJ_COND) | ||
| // | ||
| // Returns: | ||
| // true, if changes were made. | ||
| // | ||
| bool Compiler::optRedundantRelop(BasicBlock* const block) | ||
| { | ||
| Statement* const stmt = block->lastStmt(); | ||
|
|
||
| if (stmt == nullptr) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| GenTree* const jumpTree = stmt->GetRootNode(); | ||
|
|
||
| if (!jumpTree->OperIs(GT_JTRUE)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| GenTree* const tree = jumpTree->AsOp()->gtOp1; | ||
|
|
||
| if (!tree->OperIsCompare()) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // If tree has side effects other than GTF_EXCEPT, bail. | ||
| // | ||
| if ((tree->gtFlags & GTF_SIDE_EFFECT) != 0) | ||
| { | ||
| if ((tree->gtFlags & GTF_SIDE_EFFECT) != GTF_EXCEPT) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // If relop's value is known, bail. | ||
| // | ||
| const ValueNum treeVN = tree->GetVN(VNK_Liberal); | ||
|
|
||
| if (vnStore->IsVNConstant(treeVN)) | ||
| { | ||
| JITDUMP(" -- no, jump tree cond is constant\n"); | ||
| return false; | ||
| } | ||
|
|
||
| // If there's just one statement, bail. | ||
| // | ||
| if (stmt == block->firstStmt()) | ||
| { | ||
| JITDUMP(" -- no, no prior stmt\n"); | ||
| return false; | ||
| } | ||
|
|
||
| JITDUMP("\noptRedundantRelop in " FMT_BB "; jump tree is\n", block->bbNum); | ||
| DISPTREE(jumpTree); | ||
|
|
||
| // We're going to search back to find the earliest tree in block that | ||
| // * makes the current relop redundant; | ||
| // * can safely and profitably forward substituted to the jump. | ||
| // | ||
| Statement* prevStmt = stmt; | ||
| GenTree* candidateTree = nullptr; | ||
| bool reverse = false; | ||
|
|
||
| // We need to keep track of which locals might be killed by | ||
| // the trees between the expression we want to forward substitute | ||
| // and the jump. | ||
| // | ||
| // We don't use a varset here because we are indexing by local ID, | ||
| // not by tracked index. | ||
| // | ||
| // The table size here also implicitly limits how far back we'll search. | ||
| // | ||
| enum | ||
| { | ||
| DEFINED_LOCALS_SIZE = 10 | ||
| }; | ||
| unsigned definedLocals[DEFINED_LOCALS_SIZE]; | ||
| unsigned definedLocalsCount = 0; | ||
|
|
||
| while (true) | ||
| { | ||
| prevStmt = prevStmt->GetPrevStmt(); | ||
|
|
||
| // Backwards statement walks wrap around, so if we get | ||
| // back to stmt we've seen everything there is to see. | ||
| // | ||
| if (prevStmt == stmt) | ||
| { | ||
| break; | ||
| } | ||
|
|
||
| // We are looking for ASG(lcl, ...) | ||
| // | ||
| GenTree* const prevTree = prevStmt->GetRootNode(); | ||
|
|
||
| JITDUMP(" ... checking previous tree\n"); | ||
| DISPTREE(prevTree); | ||
|
|
||
| // Ignore nops. | ||
| // | ||
| if (prevTree->OperIs(GT_NOP)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| // If prevTree has side effects other than GTF_EXCEPT or GTF_ASG, bail. | ||
| // | ||
| if ((prevTree->gtFlags & GTF_SIDE_EFFECT) != (prevTree->gtFlags & (GTF_EXCEPT | GTF_ASG))) | ||
| { | ||
| JITDUMP(" -- prev tree has side effects\n"); | ||
| break; | ||
| } | ||
|
Comment on lines
+887
to
+891
There was a problem hiding this comment. It appears we only keep track of locals assigned at the root. Do we need to check |
||
|
|
||
| if (!prevTree->OperIs(GT_ASG)) | ||
| { | ||
| JITDUMP(" -- prev tree not ASG\n"); | ||
| break; | ||
| } | ||
|
|
||
| GenTree* const prevTreeLHS = prevTree->AsOp()->gtOp1; | ||
| GenTree* const prevTreeRHS = prevTree->AsOp()->gtOp2; | ||
|
|
||
| if (!prevTreeLHS->OperIs(GT_LCL_VAR)) | ||
| { | ||
| JITDUMP(" -- prev tree not ASG(LCL...)\n"); | ||
| break; | ||
| } | ||
|
|
||
| // If we are seeing PHIs we have run out of interesting stmts. | ||
| // | ||
| if (prevTreeRHS->OperIs(GT_PHI)) | ||
| { | ||
| JITDUMP(" -- prev tree is a phi\n"); | ||
| break; | ||
| } | ||
|
|
||
| // If the VN of RHS is the VN of the current tree, or is "related", consider foward sub. | ||
|
|
||
| // | ||
| // Todo: generalize to allow when normal VN of RHS == normal VN of tree, and RHS except set contains tree except | ||
| // set. | ||
| // The concern here is whether can we forward sub an excepting (but otherwise non-side effecting tree) and | ||
| // guarantee | ||
| // the same side effect behavior. | ||
| // | ||
| // A common case we see is that there's a compare of an array element guarded by a bounds check, | ||
| // so prevTree is something like: | ||
| // | ||
| // (ASG lcl, (COMMA (bds-check), (NE (array-indir), ...))) | ||
| // | ||
| // This may have the same normal VN as our tree, but also has an exception set. | ||
| // | ||
| // Another common pattern is that the prevTree contains a call. | ||
| // | ||
| const ValueNum domCmpVN = prevTreeRHS->GetVN(VNK_Liberal); | ||
| const ValueNum domCmpSwpVN = vnStore->GetRelatedRelop(domCmpVN, ValueNumStore::VN_RELATION_KIND::VRK_Swap); | ||
| const ValueNum domCmpRevVN = vnStore->GetRelatedRelop(domCmpVN, ValueNumStore::VN_RELATION_KIND::VRK_Reverse); | ||
| const ValueNum domCmpSwpRevVN = | ||
| vnStore->GetRelatedRelop(domCmpVN, ValueNumStore::VN_RELATION_KIND::VRK_SwapReverse); | ||
|
|
||
| const bool matchCmp = ((domCmpVN != ValueNumStore::NoVN) && (domCmpVN == treeVN)); | ||
| const bool matchSwp = ((domCmpSwpVN != ValueNumStore::NoVN) && (domCmpSwpVN == treeVN)); | ||
| const bool matchRev = ((domCmpRevVN != ValueNumStore::NoVN) && (domCmpRevVN == treeVN)); | ||
| const bool matchSwpRev = ((domCmpSwpRevVN != ValueNumStore::NoVN) && (domCmpSwpRevVN == treeVN)); | ||
| const bool domIsSameRelop = matchCmp || matchSwp; | ||
| const bool domIsRevRelop = matchRev || matchSwpRev; | ||
|
|
||
| // If the tree is some unrelated computation, keep looking farther up. | ||
| // | ||
| if (!domIsSameRelop && !domIsRevRelop) | ||
| { | ||
| JITDUMP(" -- prev tree VN is not related\n"); | ||
| continue; | ||
| } | ||
|
Comment on lines
+981
to
+985
There was a problem hiding this comment. I don't understand how this works. Doesn't this effectively make the interference check below useless? E.g. Foo(3, 4);
void Foo(int a, int b)
{
bool foo = a < b;
a = 15;
if (foo)
Console.WriteLine(a);
else
Console.WriteLine(b);
}15 does not have the same VN as There was a problem hiding this comment. Yeah, we should be adding |
||
|
|
||
| JITDUMP(" -- prev tree has %srelop with %s liberal VN\n", matchSwp || matchSwpRev ? "swapped " : "", | ||
| domIsSameRelop ? "the same" : "a reverse"); | ||
|
|
||
| // If lcl is not tracked, assume we can't safely reason about interference | ||
| // or liveness. | ||
| // | ||
| const unsigned prevTreeLcl = prevTreeLHS->AsLclVarCommon()->GetLclNum(); | ||
| LclVarDsc* const prevTreeLclDsc = lvaGetDesc(prevTreeLcl); | ||
|
|
||
| if (!prevTreeLclDsc->lvTracked) | ||
| { | ||
| JITDUMP(" -- prev tree defs untracked V%02u\n", prevTreeLcl); | ||
| break; | ||
| } | ||
|
|
||
| // If we've run out of room to keep track of defined locals, bail. | ||
| // | ||
| if (definedLocalsCount >= DEFINED_LOCALS_SIZE) | ||
| { | ||
| JITDUMP(" -- ran out of space for tracking kills\n"); | ||
| break; | ||
| } | ||
|
|
||
| // See if we can safely move a copy of prevTreeRHS later, to replace tree. | ||
| // We can, if none of its lcls are killed. | ||
| // | ||
| definedLocals[definedLocalsCount++] = prevTreeLcl; | ||
| bool interferes = false; | ||
|
|
||
| for (unsigned int i = 0; i < definedLocalsCount; i++) | ||
| { | ||
| if (gtHasRef(prevTreeRHS, definedLocals[i], /*def only*/ false)) | ||
| { | ||
| JITDUMP(" -- prev tree ref to V%02u interferes\n", definedLocals[i]); | ||
| interferes = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (interferes) | ||
| { | ||
| break; | ||
| } | ||
|
|
||
| // Heuristic: only forward sub a relop | ||
| // | ||
| if (!prevTreeRHS->OperIsCompare()) | ||
| { | ||
| JITDUMP(" -- prev tree is not relop\n"); | ||
| continue; | ||
| } | ||
|
|
||
| // Heuristic: if the lcl defined here is live out, forward sub | ||
| // won't result in the original becoming dead. If the local is not | ||
| // live out that still may happen, but it's less likely. | ||
| // | ||
| if (VarSetOps::IsMember(this, block->bbLiveOut, prevTreeLclDsc->lvVarIndex)) | ||
| { | ||
| JITDUMP(" -- prev tree lcl V%02u is live-out\n", prevTreeLcl); | ||
| continue; | ||
| } | ||
|
|
||
| JITDUMP(" -- prev tree is viable candidate for relop fwd sub!\n"); | ||
| candidateTree = prevTreeRHS; | ||
| reverse = domIsRevRelop; | ||
| } | ||
|
|
||
| if (candidateTree == nullptr) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Looks good -- we are going to forward-sub candidateTree | ||
| // | ||
| GenTree* const substituteTree = gtCloneExpr(candidateTree); | ||
|
|
||
| // If we need the reverse compare, make it so | ||
| // | ||
| if (reverse) | ||
| { | ||
| substituteTree->SetOper(GenTree::ReverseRelop(substituteTree->OperGet())); | ||
|
|
||
| // This substitute tree will have the VN the same as the original. | ||
| // (modulo excep sets...? hmmm) | ||
| substituteTree->SetVNsFromNode(tree); | ||
| } | ||
|
|
||
| // We just intentionally created a duplicate -- we don't want CSE to undo this. | ||
| // (hopefully, the original is now dead). | ||
| // | ||
| // Also this relop is now a subtree of a jump. | ||
| // | ||
| substituteTree->gtFlags |= (GTF_DONT_CSE | GTF_RELOP_JMP_USED); | ||
|
|
||
| gtReplaceTree(stmt, tree, substituteTree); | ||
|
There was a problem hiding this comment. Hmm, It appears here we have the use edge, so maybe just replace the operand and resequence the statement explicitly? It should be a bit cheaper I think. There was a problem hiding this comment.
Sure. |
||
| gtUpdateStmtSideEffects(stmt); | ||
|
|
||
| DEBUG_DESTROY_NODE(tree); | ||
|
|
||
| JITDUMP(" -- done! new jump tree is\n"); | ||
| DISPTREE(jumpTree); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| //------------------------------------------------------------------------ | ||
| // optReachable: see if there's a path from one block to another, | ||
| // including paths involving EH flow. | ||
|
|
||
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
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.
It might be useful if you give an example here of the type of transformations done
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.
Added one -- was that what you had in mind?
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.
That's great! Much more detailed even than what I imagined :-)