-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DomTree] Remove unnecessary domtree level check in SemiNCA (NFC) #73107
Conversation
@llvm/pr-subscribers-llvm-support Author: Nikita Popov (nikic) ChangesrunSemiNCA() currently checks that the ReverseChildren are below MinLevel in the DT, which is used when performing incremental updates. However, ReverseChildren is populated during runDFS with only the predecessors that are part of that DFS walk, which will itself be level limited in the relevant cases. As such, I don't believe that this should be checked during runSemiNCA(). This code probably dates back to a time when predecessors were not cached during runDFS and as such not limited to the visited subtree only. Full diff: https://github.com/llvm/llvm-project/pull/73107.diff 1 Files Affected:
diff --git a/llvm/include/llvm/Support/GenericDomTreeConstruction.h b/llvm/include/llvm/Support/GenericDomTreeConstruction.h
index 6564f98ab0234b0..ce893dbda27acd3 100644
--- a/llvm/include/llvm/Support/GenericDomTreeConstruction.h
+++ b/llvm/include/llvm/Support/GenericDomTreeConstruction.h
@@ -268,7 +268,7 @@ struct SemiNCAInfo {
}
// This function requires DFS to be run before calling it.
- void runSemiNCA(DomTreeT &DT, const unsigned MinLevel = 0) {
+ void runSemiNCA(DomTreeT &DT) {
const unsigned NextDFSNum(NumToNode.size());
// Initialize IDoms to spanning tree parents.
for (unsigned i = 1; i < NextDFSNum; ++i) {
@@ -289,11 +289,6 @@ struct SemiNCAInfo {
assert(NodeToInfo.contains(N) &&
"ReverseChildren should not contain unreachable predecessors");
- const TreeNodePtr TN = DT.getNode(N);
- // Skip predecessors whose level is above the subtree we are processing.
- if (TN && TN->getLevel() < MinLevel)
- continue;
-
unsigned SemiU = NodeToInfo[eval(N, i + 1, EvalStack)].Semi;
if (SemiU < WInfo.Semi) WInfo.Semi = SemiU;
}
@@ -998,7 +993,7 @@ struct SemiNCAInfo {
SemiNCAInfo SNCA(BUI);
SNCA.runDFS(ToIDom, 0, DescendBelow, 0);
LLVM_DEBUG(dbgs() << "\tRunning Semi-NCA\n");
- SNCA.runSemiNCA(DT, Level);
+ SNCA.runSemiNCA(DT);
SNCA.reattachExistingSubtree(DT, PrevIDomSubTree);
}
@@ -1122,7 +1117,7 @@ struct SemiNCAInfo {
<< BlockNamePrinter(PrevIDom) << "\nRunning Semi-NCA\n");
// Rebuild the remaining part of affected subtree.
- SNCA.runSemiNCA(DT, MinLevel);
+ SNCA.runSemiNCA(DT);
SNCA.reattachExistingSubtree(DT, PrevIDom);
}
|
if (TN && TN->getLevel() < MinLevel) | ||
continue; |
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.
Do we have some data to confirm this branch is dynamically dead? IIUC, this patch relies on tis being the case.
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.
I checked that if I replace this with an assertion, it does not fire in our test suite.
I opted not to keep the assertion, because this completely removes the dependency of runSemiNCA() on the dominator tree, which is imho cleaner.
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.
I think @alinas might have access to some fancy fleet-wide profiles -- could you confirm if this appears dead?
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.
LGTM, but I would be more confident if we could wait for a confirmation that this if
body is dead in some fleed-wide profile.
runSemiNCA() currently checks that the ReverseChildren are below MinLevel in the DT, which is used when performing incremental updates. However, ReverseChildren is populated during runDFS with only the predecessors that are part of that DFS walk, which will itself be level limited in the relevant cases. As such, I don't believe that this should be checked during runSemiNCA(). This code probably dates back to a time when predecessors were run cached during runDFS and as such not limited to the visited subtree only.
runSemiNCA() currently checks that the ReverseChildren are below MinLevel in the DT, which is used when performing incremental updates.
However, ReverseChildren is populated during runDFS with only the predecessors that are part of that DFS walk, which will itself be level limited in the relevant cases. As such, I don't believe that this should be checked during runSemiNCA().
This code probably dates back to a time when predecessors were not cached during runDFS and as such not limited to the visited subtree only.