[llvm] c992690 - Revert "[nfc][mlgo] Incrementally update DominatorTreeAnalysis in FunctionPropertiesAnalysis (#104867)"
Hans Wennborg via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 27 07:04:47 PDT 2024
Author: Hans Wennborg
Date: 2024-08-27T16:04:26+02:00
New Revision: c992690179eb5de6efe47d5c8f3a23f2302723f2
URL: https://github.com/llvm/llvm-project/commit/c992690179eb5de6efe47d5c8f3a23f2302723f2
DIFF: https://github.com/llvm/llvm-project/commit/c992690179eb5de6efe47d5c8f3a23f2302723f2.diff
LOG: Revert "[nfc][mlgo] Incrementally update DominatorTreeAnalysis in FunctionPropertiesAnalysis (#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 a2a5508bdae7d115b6c3ace461beb7a987a44407 and the
follow-up commit cdd11d694a406a98a16d6265168ee2fbe1b6a87c.
Added:
Modified:
llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h
llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp
llvm/lib/Analysis/MLInlineAdvisor.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h b/llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h
index af72f6e0f90b11..ee447d3e4ebb6a 100644
--- a/llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h
+++ b/llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h
@@ -15,7 +15,6 @@
#define LLVM_ANALYSIS_FUNCTIONPROPERTIESANALYSIS_H
#include "llvm/ADT/DenseSet.h"
-#include "llvm/IR/Dominators.h"
#include "llvm/IR/PassManager.h"
namespace llvm {
@@ -187,12 +186,7 @@ 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 07906fa1aa6c65..6d6ec6c7b1cc76 100644
--- a/llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp
+++ b/llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp
@@ -326,14 +326,6 @@ 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
@@ -344,11 +336,6 @@ 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).
@@ -369,30 +356,6 @@ FunctionPropertiesUpdater::FunctionPropertiesUpdater(
FPI.updateForBB(*BB, -1);
}
-DominatorTree &FunctionPropertiesUpdater::getUpdatedDominatorTree(
- FunctionAnalysisManager &FAM) const {
- auto &DT =
- FAM.getResult<DominatorTreeAnalysis>(const_cast<Function &>(Caller));
-
- SmallVector<DominatorTree::UpdateType, 2> FinalDomTreeUpdates;
-
- for (auto &Upd : DomTreeUpdates)
- FinalDomTreeUpdates.push_back(Upd);
-
- DenseSet<const BasicBlock *> Inserted;
- for (auto *Succ : successors(&CallSiteBB))
- if (Inserted.insert(Succ).second)
- FinalDomTreeUpdates.push_back({DominatorTree::UpdateKind::Insert,
- const_cast<BasicBlock *>(&CallSiteBB),
- const_cast<BasicBlock *>(Succ)});
-
- DT.applyUpdates(FinalDomTreeUpdates);
-#ifdef EXPENSIVE_CHECKS
- assert(DT.verify(DominatorTree::VerificationLevel::Full));
-#endif
- 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
@@ -420,7 +383,8 @@ void FunctionPropertiesUpdater::finish(FunctionAnalysisManager &FAM) const {
// remove E.
SetVector<const BasicBlock *> Reinclude;
SetVector<const BasicBlock *> Unreachable;
- auto &DT = getUpdatedDominatorTree(FAM);
+ const auto &DT =
+ FAM.getResult<DominatorTreeAnalysis>(const_cast<Function &>(Caller));
if (&CallSiteBB != &*Caller.begin())
Reinclude.insert(&*Caller.begin());
@@ -463,9 +427,6 @@ void FunctionPropertiesUpdater::finish(FunctionAnalysisManager &FAM) const {
const auto &LI = FAM.getResult<LoopAnalysis>(const_cast<Function &>(Caller));
FPI.updateAggregateStats(Caller, LI);
-#ifdef EXPENSIVE_CHECKS
- assert(isUpdateValid(Caller, FPI, FAM));
-#endif
}
bool FunctionPropertiesUpdater::isUpdateValid(Function &F,
diff --git a/llvm/lib/Analysis/MLInlineAdvisor.cpp b/llvm/lib/Analysis/MLInlineAdvisor.cpp
index 8bb5efcf1b2ecb..b59aa4810005bc 100644
--- a/llvm/lib/Analysis/MLInlineAdvisor.cpp
+++ b/llvm/lib/Analysis/MLInlineAdvisor.cpp
@@ -288,6 +288,7 @@ void MLInlineAdvisor::onSuccessfulInlining(const MLInlineAdvice &Advice,
{
PreservedAnalyses PA = PreservedAnalyses::all();
PA.abandon<FunctionPropertiesAnalysis>();
+ PA.abandon<DominatorTreeAnalysis>();
PA.abandon<LoopAnalysis>();
FAM.invalidate(*Caller, PA);
}
More information about the llvm-commits
mailing list