[llvm] Reapply "[nfc][mlgo] Incrementally update DominatorTreeAnalysis in FunctionPropertiesAnalysis (#104867) (PR #106309)
Mircea Trofin via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 27 16:29:34 PDT 2024
https://github.com/mtrofin created https://github.com/llvm/llvm-project/pull/106309
Reverts c992690179eb5de6efe47d5c8f3a23f2302723f2.
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.
>From e407aa62fe474d8084f52183fb02aed81c408a84 Mon Sep 17 00:00:00 2001
From: Mircea Trofin <mtrofin at google.com>
Date: Tue, 27 Aug 2024 14:01:49 -0700
Subject: [PATCH] Reapply "[nfc][mlgo] Incrementally update
DominatorTreeAnalysis in FunctionPropertiesAnalysis (#104867)
Reverts c992690179eb5de6efe47d5c8f3a23f2302723f2.
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.
---
.../Analysis/FunctionPropertiesAnalysis.h | 6 ++
.../Analysis/FunctionPropertiesAnalysis.cpp | 50 +++++++++++++-
llvm/lib/Analysis/MLInlineAdvisor.cpp | 1 -
.../FunctionPropertiesAnalysisTest.cpp | 67 +++++++++++++++++++
4 files changed, 121 insertions(+), 3 deletions(-)
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..c2746e6cecb59b 100644
--- a/llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp
+++ b/llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp
@@ -326,6 +326,20 @@ 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.
+ // Some BBs have (at this point) duplicate edges. Remove duplicates, otherwise
+ // the DT updater will not apply changes correctly.
+ DenseSet<const BasicBlock *> Inserted;
+ for (auto *Succ : successors(&CallSiteBB))
+ if (Inserted.insert(Succ).second)
+ DomTreeUpdates.emplace_back(DominatorTree::UpdateKind::Delete,
+ const_cast<BasicBlock *>(&CallSiteBB),
+ const_cast<BasicBlock *>(Succ));
+ // Reuse Inserted (which has some allocated capacity at this point) below, if
+ // we have an invoke.
+ Inserted.clear();
// 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 +350,12 @@ 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))
+ if (Inserted.insert(Succ).second)
+ 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 +376,30 @@ 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
@@ -383,8 +427,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());
@@ -427,6 +470,9 @@ 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 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);
}
diff --git a/llvm/unittests/Analysis/FunctionPropertiesAnalysisTest.cpp b/llvm/unittests/Analysis/FunctionPropertiesAnalysisTest.cpp
index 3b3823589437a8..d0eb397d6c2c07 100644
--- a/llvm/unittests/Analysis/FunctionPropertiesAnalysisTest.cpp
+++ b/llvm/unittests/Analysis/FunctionPropertiesAnalysisTest.cpp
@@ -999,4 +999,71 @@ declare <4 x ptr> @f4()
EXPECT_EQ(DetailedF1Properties.CallReturnsVectorPointerCount, 1);
EnableDetailedFunctionProperties.setValue(false);
}
+
+TEST_F(FunctionPropertiesAnalysisTest, ReAddEdges) {
+ LLVMContext C;
+ std::unique_ptr<Module> M = makeLLVMModule(C, R"IR(
+define hidden void @f1(ptr noundef %destatep, i32 noundef %offset, i8 noundef zeroext %byte1) {
+entry:
+ %cmp = icmp eq i8 %byte1, 0
+ br i1 %cmp, label %if.then, label %if.else
+
+if.then: ; preds = %entry
+ call fastcc void @f2(ptr noundef %destatep, i32 noundef 37, i32 noundef 600)
+ %and = and i32 %offset, 3
+ switch i32 %and, label %default.unreachable [
+ i32 0, label %sw.bb
+ i32 1, label %sw.bb1
+ i32 2, label %sw.bb1
+ i32 3, label %if.end
+ ]
+
+sw.bb: ; preds = %if.then
+ call fastcc void @f2(ptr noundef %destatep, i32 noundef 57, i32 noundef 600)
+ br label %if.end
+
+sw.bb1: ; preds = %if.then, %if.then
+ call fastcc void @f2(ptr noundef %destatep, i32 noundef 56, i32 noundef 600) #34
+ br label %if.end
+
+default.unreachable: ; preds = %if.then
+ unreachable
+
+if.else: ; preds = %entry
+ call fastcc void @f2(ptr noundef %destatep, i32 noundef 56, i32 noundef 600)
+ br label %if.end
+
+if.end: ; preds = %sw.bb, %sw.bb1, %if.then, %if.else
+ ret void
+}
+
+define internal fastcc void @f2(ptr nocapture noundef %destatep, i32 noundef %r_enc, i32 noundef %whack) {
+entry:
+ %enc_prob = getelementptr inbounds nuw i8, ptr %destatep, i32 512
+ %arrayidx = getelementptr inbounds [67 x i32], ptr %enc_prob, i32 0, i32 %r_enc
+ %0 = load i32, ptr %arrayidx, align 4
+ %sub = sub nsw i32 %0, %whack
+ store i32 %sub, ptr %arrayidx, align 4
+ ret void
+}
+ )IR");
+ auto *F1 = M->getFunction("f1");
+ auto *F2 = M->getFunction("f2");
+ auto *CB = [&]() -> CallBase * {
+ for (auto &BB : *F1)
+ for (auto &I : BB)
+ if (auto *CB = dyn_cast<CallBase>(&I);
+ CB && CB->getCalledFunction() && CB->getCalledFunction() == F2)
+ return CB;
+ return nullptr;
+ }();
+ ASSERT_NE(CB, nullptr);
+ auto FPI = buildFPI(*F1);
+ FunctionPropertiesUpdater FPU(FPI, *CB);
+ InlineFunctionInfo IFI;
+ auto IR = llvm::InlineFunction(*CB, IFI);
+ EXPECT_TRUE(IR.isSuccess());
+ invalidate(*F1);
+ EXPECT_TRUE(FPU.finishAndTest(FAM));
+}
} // end anonymous namespace
More information about the llvm-commits
mailing list