[nfc][mlgo] Incrementally update DominatorTreeAnalysis in FunctionPropertiesAnalysis #104867
Conversation
This file contains hidden or 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 stack of pull requests is managed by Graphite. Learn more about stacking. |
mtrofin
changed the title
llvmbot
added
mlgo
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis Author: Mircea Trofin (mtrofin) ChangesWe need the dominator tree analysis for loop info analysis, which we need to get features like most nested loop and number of top level loops. Invalidating and recomputing these from scratch after each successful inlining can sometimes lead to lengthy compile times. We don't need to recompute from scratch, though, since we have some boundary information about where the changes to the CFG happen; moreover, for dom tree, the API supports incrementally updating the analysis result. This change addresses the dom tree part. The loop info is still recomputed from scratch. This does reduce the compile time quite significantly already, though (~5x in a specific case) The loop info change might be more involved and would follow in a subsequent PR. Full diff: https://github.com/llvm/llvm-project/pull/104867.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h b/llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h
index ee447d3e4ebb6a..af72f6e0f90b11 100644
--- a/llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h
+++ b/llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h
@@ -15,6 +15,7 @@
#define LLVM_ANALYSIS_FUNCTIONPROPERTIESANALYSIS_H
#include "llvm/ADT/DenseSet.h"
+#include "llvm/IR/Dominators.h"
#include "llvm/IR/PassManager.h"
namespace llvm {
@@ -186,7 +187,12 @@ class FunctionPropertiesUpdater {
static bool isUpdateValid(Function &F, const FunctionPropertiesInfo &FPI,
FunctionAnalysisManager &FAM);
+ DominatorTree &getUpdatedDominatorTree(FunctionAnalysisManager &FAM) const;
+
DenseSet<const BasicBlock *> Successors;
+
+ // Edges we might potentially need to remove from the dominator tree.
+ SmallVector<DominatorTree::UpdateType, 2> DomTreeUpdates;
};
} // namespace llvm
#endif // LLVM_ANALYSIS_FUNCTIONPROPERTIESANALYSIS_H
diff --git a/llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp b/llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp
index 6d6ec6c7b1cc76..cce7c08cf35e8e 100644
--- a/llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp
+++ b/llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp
@@ -326,6 +326,14 @@ FunctionPropertiesUpdater::FunctionPropertiesUpdater(
// with the CB BB ('Entry') between which the inlined callee will be pasted.
Successors.insert(succ_begin(&CallSiteBB), succ_end(&CallSiteBB));
+ // the outcome of the inlining may be that some edges get lost (DCEd BBs
+ // because inlining brought some constant, for example). We don't know which
+ // edges will be removed, so we list all of them as potentially removable.
+ for (auto *Succ : successors(&CallSiteBB))
+ DomTreeUpdates.emplace_back(DominatorTree::UpdateKind::Delete,
+ const_cast<BasicBlock *>(&CallSiteBB),
+ const_cast<BasicBlock *>(Succ));
+
// Inlining only handles invoke and calls. If this is an invoke, and inlining
// it pulls another invoke, the original landing pad may get split, so as to
// share its content with other potential users. So the edge up to which we
@@ -336,6 +344,11 @@ FunctionPropertiesUpdater::FunctionPropertiesUpdater(
if (const auto *II = dyn_cast<InvokeInst>(&CB)) {
const auto *UnwindDest = II->getUnwindDest();
Successors.insert(succ_begin(UnwindDest), succ_end(UnwindDest));
+ // Same idea as above, we pretend we lose all these edges.
+ for (auto *Succ : successors(UnwindDest))
+ DomTreeUpdates.emplace_back(DominatorTree::UpdateKind::Delete,
+ const_cast<BasicBlock *>(UnwindDest),
+ const_cast<BasicBlock *>(Succ));
}
// Exclude the CallSiteBB, if it happens to be its own successor (1-BB loop).
@@ -356,6 +369,40 @@ FunctionPropertiesUpdater::FunctionPropertiesUpdater(
FPI.updateForBB(*BB, -1);
}
+DominatorTree &FunctionPropertiesUpdater::getUpdatedDominatorTree(
+ FunctionAnalysisManager &FAM) const {
+ auto &DT =
+ FAM.getResult<DominatorTreeAnalysis>(const_cast<Function &>(Caller));
+
+ DenseSet<const BasicBlock *> NewSucc;
+ NewSucc.insert(succ_begin(&CallSiteBB), succ_end(&CallSiteBB));
+
+ // tell the DomTree about the new edges
+ std::deque<const BasicBlock *> Worklist;
+ Worklist.push_back(&CallSiteBB);
+ while (!Worklist.empty()) {
+ auto *BB = Worklist.front();
+ Worklist.pop_front();
+ assert(DT.getNode(BB));
+
+ for (auto *Succ : NewSucc) {
+ if (!DT.getNode(Succ))
+ Worklist.push_back(Succ);
+ DT.insertEdge(const_cast<BasicBlock *>(BB),
+ const_cast<BasicBlock *>(Succ));
+ }
+ }
+ // Build the list of edges to actually remove. Those are those edges in the
+ // DomTreeUpdates that cannot be found in the CFG anymore.
+ SmallVector<DominatorTree::UpdateType, 2> FinalDomTreeUpdates;
+ for (auto &Upd : DomTreeUpdates)
+ if (!llvm::is_contained(successors(Upd.getFrom()), Upd.getTo()))
+ FinalDomTreeUpdates.push_back(Upd);
+
+ DT.applyUpdates(FinalDomTreeUpdates);
+ return DT;
+}
+
void FunctionPropertiesUpdater::finish(FunctionAnalysisManager &FAM) const {
// Update feature values from the BBs that were copied from the callee, or
// might have been modified because of inlining. The latter have been
@@ -383,8 +430,7 @@ void FunctionPropertiesUpdater::finish(FunctionAnalysisManager &FAM) const {
// remove E.
SetVector<const BasicBlock *> Reinclude;
SetVector<const BasicBlock *> Unreachable;
- const auto &DT =
- FAM.getResult<DominatorTreeAnalysis>(const_cast<Function &>(Caller));
+ auto &DT = getUpdatedDominatorTree(FAM);
if (&CallSiteBB != &*Caller.begin())
Reinclude.insert(&*Caller.begin());
diff --git a/llvm/lib/Analysis/MLInlineAdvisor.cpp b/llvm/lib/Analysis/MLInlineAdvisor.cpp
index b59aa4810005bc..8bb5efcf1b2ecb 100644
--- a/llvm/lib/Analysis/MLInlineAdvisor.cpp
+++ b/llvm/lib/Analysis/MLInlineAdvisor.cpp
@@ -288,7 +288,6 @@ void MLInlineAdvisor::onSuccessfulInlining(const MLInlineAdvice &Advice,
{
PreservedAnalyses PA = PreservedAnalyses::all();
PA.abandon<FunctionPropertiesAnalysis>();
- PA.abandon<DominatorTreeAnalysis>();
PA.abandon<LoopAnalysis>();
FAM.invalidate(*Caller, PA);
}
|
|
@llvm/pr-subscribers-mlgo Author: Mircea Trofin (mtrofin) ChangesWe need the dominator tree analysis for loop info analysis, which we need to get features like most nested loop and number of top level loops. Invalidating and recomputing these from scratch after each successful inlining can sometimes lead to lengthy compile times. We don't need to recompute from scratch, though, since we have some boundary information about where the changes to the CFG happen; moreover, for dom tree, the API supports incrementally updating the analysis result. This change addresses the dom tree part. The loop info is still recomputed from scratch. This does reduce the compile time quite significantly already, though (~5x in a specific case) The loop info change might be more involved and would follow in a subsequent PR. Full diff: https://github.com/llvm/llvm-project/pull/104867.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h b/llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h
index ee447d3e4ebb6a..af72f6e0f90b11 100644
--- a/llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h
+++ b/llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h
@@ -15,6 +15,7 @@
#define LLVM_ANALYSIS_FUNCTIONPROPERTIESANALYSIS_H
#include "llvm/ADT/DenseSet.h"
+#include "llvm/IR/Dominators.h"
#include "llvm/IR/PassManager.h"
namespace llvm {
@@ -186,7 +187,12 @@ class FunctionPropertiesUpdater {
static bool isUpdateValid(Function &F, const FunctionPropertiesInfo &FPI,
FunctionAnalysisManager &FAM);
+ DominatorTree &getUpdatedDominatorTree(FunctionAnalysisManager &FAM) const;
+
DenseSet<const BasicBlock *> Successors;
+
+ // Edges we might potentially need to remove from the dominator tree.
+ SmallVector<DominatorTree::UpdateType, 2> DomTreeUpdates;
};
} // namespace llvm
#endif // LLVM_ANALYSIS_FUNCTIONPROPERTIESANALYSIS_H
diff --git a/llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp b/llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp
index 6d6ec6c7b1cc76..cce7c08cf35e8e 100644
--- a/llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp
+++ b/llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp
@@ -326,6 +326,14 @@ FunctionPropertiesUpdater::FunctionPropertiesUpdater(
// with the CB BB ('Entry') between which the inlined callee will be pasted.
Successors.insert(succ_begin(&CallSiteBB), succ_end(&CallSiteBB));
+ // the outcome of the inlining may be that some edges get lost (DCEd BBs
+ // because inlining brought some constant, for example). We don't know which
+ // edges will be removed, so we list all of them as potentially removable.
+ for (auto *Succ : successors(&CallSiteBB))
+ DomTreeUpdates.emplace_back(DominatorTree::UpdateKind::Delete,
+ const_cast<BasicBlock *>(&CallSiteBB),
+ const_cast<BasicBlock *>(Succ));
+
// Inlining only handles invoke and calls. If this is an invoke, and inlining
// it pulls another invoke, the original landing pad may get split, so as to
// share its content with other potential users. So the edge up to which we
@@ -336,6 +344,11 @@ FunctionPropertiesUpdater::FunctionPropertiesUpdater(
if (const auto *II = dyn_cast<InvokeInst>(&CB)) {
const auto *UnwindDest = II->getUnwindDest();
Successors.insert(succ_begin(UnwindDest), succ_end(UnwindDest));
+ // Same idea as above, we pretend we lose all these edges.
+ for (auto *Succ : successors(UnwindDest))
+ DomTreeUpdates.emplace_back(DominatorTree::UpdateKind::Delete,
+ const_cast<BasicBlock *>(UnwindDest),
+ const_cast<BasicBlock *>(Succ));
}
// Exclude the CallSiteBB, if it happens to be its own successor (1-BB loop).
@@ -356,6 +369,40 @@ FunctionPropertiesUpdater::FunctionPropertiesUpdater(
FPI.updateForBB(*BB, -1);
}
+DominatorTree &FunctionPropertiesUpdater::getUpdatedDominatorTree(
+ FunctionAnalysisManager &FAM) const {
+ auto &DT =
+ FAM.getResult<DominatorTreeAnalysis>(const_cast<Function &>(Caller));
+
+ DenseSet<const BasicBlock *> NewSucc;
+ NewSucc.insert(succ_begin(&CallSiteBB), succ_end(&CallSiteBB));
+
+ // tell the DomTree about the new edges
+ std::deque<const BasicBlock *> Worklist;
+ Worklist.push_back(&CallSiteBB);
+ while (!Worklist.empty()) {
+ auto *BB = Worklist.front();
+ Worklist.pop_front();
+ assert(DT.getNode(BB));
+
+ for (auto *Succ : NewSucc) {
+ if (!DT.getNode(Succ))
+ Worklist.push_back(Succ);
+ DT.insertEdge(const_cast<BasicBlock *>(BB),
+ const_cast<BasicBlock *>(Succ));
+ }
+ }
+ // Build the list of edges to actually remove. Those are those edges in the
+ // DomTreeUpdates that cannot be found in the CFG anymore.
+ SmallVector<DominatorTree::UpdateType, 2> FinalDomTreeUpdates;
+ for (auto &Upd : DomTreeUpdates)
+ if (!llvm::is_contained(successors(Upd.getFrom()), Upd.getTo()))
+ FinalDomTreeUpdates.push_back(Upd);
+
+ DT.applyUpdates(FinalDomTreeUpdates);
+ return DT;
+}
+
void FunctionPropertiesUpdater::finish(FunctionAnalysisManager &FAM) const {
// Update feature values from the BBs that were copied from the callee, or
// might have been modified because of inlining. The latter have been
@@ -383,8 +430,7 @@ void FunctionPropertiesUpdater::finish(FunctionAnalysisManager &FAM) const {
// remove E.
SetVector<const BasicBlock *> Reinclude;
SetVector<const BasicBlock *> Unreachable;
- const auto &DT =
- FAM.getResult<DominatorTreeAnalysis>(const_cast<Function &>(Caller));
+ auto &DT = getUpdatedDominatorTree(FAM);
if (&CallSiteBB != &*Caller.begin())
Reinclude.insert(&*Caller.begin());
diff --git a/llvm/lib/Analysis/MLInlineAdvisor.cpp b/llvm/lib/Analysis/MLInlineAdvisor.cpp
index b59aa4810005bc..8bb5efcf1b2ecb 100644
--- a/llvm/lib/Analysis/MLInlineAdvisor.cpp
+++ b/llvm/lib/Analysis/MLInlineAdvisor.cpp
@@ -288,7 +288,6 @@ void MLInlineAdvisor::onSuccessfulInlining(const MLInlineAdvice &Advice,
{
PreservedAnalyses PA = PreservedAnalyses::all();
PA.abandon<FunctionPropertiesAnalysis>();
- PA.abandon<DominatorTreeAnalysis>();
PA.abandon<LoopAnalysis>();
FAM.invalidate(*Caller, PA);
}
|
mtrofin
force-pushed
the
users/mtrofin/08-19-_nfc_incrementally_update_dominatortreeanalysis_in_functionpropertiesanalysis
branch
from
a702a49 to
67295e1
Compare
|
The patch doesn't have new tests, but is presumably exercised by all the inliner tests. Can you remind us how the dominator tree analysis is checked for correctness? Why should we be confident that the new dominator tree is correct? Is it validated in regular +asserts builds, or is it covered under expensive checks? Have you run the test suite with that validation ahead of time, or which bot would we monitor post submit to see if it comes back green? |
DT does not get verified by default, at least in this pipeline position. You need to add explicit DT.verify() calls for that (the way this is often done is to use fast verification in assertion builds and full verification under EXPENSIVE_CHECKS). |
nikic
reviewed
Aug 21, 2024
| auto &DT = | ||
| FAM.getResult<DominatorTreeAnalysis>(const_cast<Function &>(Caller)); | ||
|
|
||
| DenseSet<const BasicBlock *> NewSucc; |
There was a problem hiding this comment.
Needs to be SetVector, you'll get nondet results otherwise.
| if (!DT.getNode(Succ)) | ||
| Worklist.push_back(Succ); | ||
| DT.insertEdge(const_cast<BasicBlock *>(BB), | ||
| const_cast<BasicBlock *>(Succ)); |
There was a problem hiding this comment.
Should collect updates here instead of calling insertEdge directly?
There was a problem hiding this comment.
true, and it's more consistent. Done.
There was a problem hiding this comment.
I actually addressed this feedback incorrectly, and the original code was kind of nonsensical here (the working list was a bit... wat).
I believe cdd11d6 fixed it all, but please let me know if there are any concerns.
Adding |
mtrofin
force-pushed
the
users/mtrofin/08-19-_nfc_incrementally_update_dominatortreeanalysis_in_functionpropertiesanalysis
branch
from
67295e1 to
a361f13
Compare
Oh, forgot to finish the statment... the assert got moved to an explicit test during unittests, |
mtrofin
force-pushed
the
users/mtrofin/08-19-_nfc_incrementally_update_dominatortreeanalysis_in_functionpropertiesanalysis
branch
from
a361f13 to
584cfcc
Compare
alinas
reviewed
Aug 21, 2024
Yes, afaik
AFAIK, DT / LI aren't used for the non-ML InlineCost calculation, but indeed, we could use their patch-ability to keep them "fresh" during inlining and avoid recalculating them, if that becomes an issue in some scenarios. |
mtrofin
force-pushed
the
users/mtrofin/08-19-_nfc_incrementally_update_dominatortreeanalysis_in_functionpropertiesanalysis
branch
from
584cfcc to
25f1534
Compare
mtrofin
force-pushed
the
users/mtrofin/08-19-_nfc_incrementally_update_dominatortreeanalysis_in_functionpropertiesanalysis
branch
from
25f1534 to
c5e48e0
Compare
mtrofin
deleted the
users/mtrofin/08-19-_nfc_incrementally_update_dominatortreeanalysis_in_functionpropertiesanalysis
branch
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/3964 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/175/builds/3922 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/185/builds/3951 Here is the relevant piece of the build log for the reference: |
mtrofin
added a commit
that referenced
this pull request
Aug 23, 2024
An assert was left over after addressing feedback. In the process of fixing, realized the way I addressed the feedback was also incomplete.
|
We're hitting assertions which seem related to this, see https://crbug.com/362296600 for details. I'll prepare a revert for now. |
zmodem
added a commit
that referenced
this pull request
Aug 27, 2024
…ctionPropertiesAnalysis (#104867)" This seems to cause asserts in our builds: llvm/include/llvm/Support/GenericDomTreeConstruction.h:927: static void llvm::DomTreeBuilder::SemiNCAInfo<llvm::DominatorTreeBase<BasicBlock, false>>::DeleteEdge(DomTreeT &, const BatchUpdatePtr, const NodePtr, const NodePtr) [DomTreeT = llvm::DominatorTreeBase<BasicBlock, false>]: Assertion `!IsSuccessor(To, From) && "Deleted edge still exists in the CFG!"' failed. and llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:390: DominatorTree &llvm::FunctionPropertiesUpdater::getUpdatedDominatorTree(FunctionAnalysisManager &) const: Assertion `DT.getNode(BB)' failed. See comment on the PR. > We need the dominator tree analysis for loop info analysis, which we need to get features like most nested loop and number of top level loops. Invalidating and recomputing these from scratch after each successful inlining can sometimes lead to lengthy compile times. We don't need to recompute from scratch, though, since we have some boundary information about where the changes to the CFG happen; moreover, for dom tree, the API supports incrementally updating the analysis result. > > This change addresses the dom tree part. The loop info is still recomputed from scratch. This does reduce the compile time quite significantly already, though (~5x in a specific case) > > The loop info change might be more involved and would follow in a subsequent PR. This reverts commit a2a5508 and the follow-up commit cdd11d6.
mtrofin
added a commit
that referenced
this pull request
Aug 27, 2024
…nctionPropertiesAnalysis (#104867)
mtrofin
added a commit
to mtrofin/llvm-project
that referenced
this pull request
Aug 27, 2024
…nctionPropertiesAnalysis (llvm#104867) Reverts c992690. The problem is that if there is a sequence "{delete A->B} {delete A->B} {insert A->B}" the net result is "{delete A->B}", which is not what we want. The fix is to sanitize the list of deletes, just like we do for inserts.
mtrofin
added a commit
that referenced
this pull request
Aug 30, 2024
…nctionPropertiesAnalysis (#104867) (#106309) Reverts c992690. The problem is that if there is a sequence "{delete A->B} {delete A->B} {insert A->B}" the net result is "{delete A->B}", which is not what we want. Duplicate successors may happen in cases like switch statements (as shown in the unit test). The second problem was that in `invoke` cases, some edges we speculate may get deleted don't, but are also not reachable from the inlined call site's basic block. We just need to check which edges are actually not present anymore. The fix is to sanitize the list of deletes, just like we do for inserts.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We need the dominator tree analysis for loop info analysis, which we need to get features like most nested loop and number of top level loops. Invalidating and recomputing these from scratch after each successful inlining can sometimes lead to lengthy compile times. We don't need to recompute from scratch, though, since we have some boundary information about where the changes to the CFG happen; moreover, for dom tree, the API supports incrementally updating the analysis result.
This change addresses the dom tree part. The loop info is still recomputed from scratch. This does reduce the compile time quite significantly already, though (~5x in a specific case)
The loop info change might be more involved and would follow in a subsequent PR.