[llvm] b1f4168 - [IPSCCP] Decouple queries for function analysis results.
Alexandros Lamprineas via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 1 08:41:33 PDT 2023
Author: Alexandros Lamprineas
Date: 2023-06-01T16:38:04+01:00
New Revision: b1f41685a60e5416af8f636393bffd03ac4c13f5
URL: https://github.com/llvm/llvm-project/commit/b1f41685a60e5416af8f636393bffd03ac4c13f5
DIFF: https://github.com/llvm/llvm-project/commit/b1f41685a60e5416af8f636393bffd03ac4c13f5.diff
LOG: [IPSCCP] Decouple queries for function analysis results.
The SCCPSolver is using a structure (AnalysisResultsForFn) where it keeps
pointers to various analyses needed by the IPSCCP pass. These analyses are
requested all at the same time, which can become problematic in some cases.
For example one could be retrieved via getCachedAnalysis() prior to the
actual execution of the analysis. In more detail:
The IPSCCP pass uses a DomTreeUpdater to preserve the PostDominatorTree
in case the PostDominatorTreeAnalysis had run before IPSCCP. Starting with
commit 1b1232047e83b the IPSCCP pass may use BlockFrequencyAnalysis for
some functions in the module. As a result, the PostDominatorTreeAnalysis
may not run until the BlockFrequencyAnalysis has run, since the latter
analysis depends on the former. Currently, we setup the DomTreeUpdater
using getCachedAnalysis to retrieve a PostDominatorTree. This happens
before BlockFrequencyAnalysis has run, therefore the cached analysis can
become invalid by the time we use it.
Differential Revision: https://reviews.llvm.org/D151666
Added:
llvm/test/Transforms/SCCP/ipsccp-preserve-pdt.ll
Modified:
llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h
llvm/include/llvm/Transforms/Utils/SCCPSolver.h
llvm/lib/Transforms/IPO/SCCP.cpp
llvm/lib/Transforms/Utils/SCCPSolver.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h b/llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h
index e37386c85cfe6..32c65cfc331b4 100644
--- a/llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h
+++ b/llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h
@@ -145,8 +145,6 @@ class FunctionSpecializer {
~FunctionSpecializer();
- bool isClonedFunction(Function *F) { return Specializations.count(F); }
-
bool run();
private:
diff --git a/llvm/include/llvm/Transforms/Utils/SCCPSolver.h b/llvm/include/llvm/Transforms/Utils/SCCPSolver.h
index cf3c3b7eee49f..16462c7ecef29 100644
--- a/llvm/include/llvm/Transforms/Utils/SCCPSolver.h
+++ b/llvm/include/llvm/Transforms/Utils/SCCPSolver.h
@@ -39,14 +39,6 @@ class TargetLibraryInfo;
class Value;
class ValueLatticeElement;
-/// Helper struct for bundling up the analysis results per function for IPSCCP.
-struct AnalysisResultsForFn {
- std::unique_ptr<PredicateInfo> PredInfo;
- DominatorTree *DT;
- PostDominatorTree *PDT;
- LoopInfo *LI;
-};
-
/// Helper struct shared between Function Specialization and SCCP Solver.
struct ArgInfo {
Argument *Formal; // The Formal argument being analysed.
@@ -82,7 +74,9 @@ class SCCPSolver {
~SCCPSolver();
- void addAnalysis(Function &F, AnalysisResultsForFn A);
+ void addLoopInfo(Function &F, LoopInfo &LI);
+
+ void addPredicateInfo(Function &F, DominatorTree &DT, AssumptionCache &AC);
/// markBlockExecutable - This method can be used by clients to mark all of
/// the blocks that are known to be intrinsically live in the processed unit.
@@ -93,8 +87,6 @@ class SCCPSolver {
const LoopInfo &getLoopInfo(Function &F);
- DomTreeUpdater getDTU(Function &F);
-
/// trackValueOfGlobalVariable - Clients can use this method to
/// inform the SCCPSolver that it should track loads and stores to the
/// specified global variable if it can. This is only legal to call if
diff --git a/llvm/lib/Transforms/IPO/SCCP.cpp b/llvm/lib/Transforms/IPO/SCCP.cpp
index 5e2a23b9e62df..5d0a2afd0392d 100644
--- a/llvm/lib/Transforms/IPO/SCCP.cpp
+++ b/llvm/lib/Transforms/IPO/SCCP.cpp
@@ -110,7 +110,8 @@ static bool runIPSCCP(
std::function<const TargetLibraryInfo &(Function &)> GetTLI,
std::function<TargetTransformInfo &(Function &)> GetTTI,
std::function<AssumptionCache &(Function &)> GetAC,
- function_ref<AnalysisResultsForFn(Function &)> getAnalysis,
+ std::function<DominatorTree &(Function &)> GetDT,
+ std::function<LoopInfo &(Function &)> GetLI,
bool IsFuncSpecEnabled) {
SCCPSolver Solver(DL, GetTLI, M.getContext());
FunctionSpecializer Specializer(Solver, M, FAM, GetTLI, GetTTI, GetAC);
@@ -121,7 +122,12 @@ static bool runIPSCCP(
if (F.isDeclaration())
continue;
- Solver.addAnalysis(F, getAnalysis(F));
+ DominatorTree &DT = GetDT(F);
+ AssumptionCache &AC = GetAC(F);
+ Solver.addPredicateInfo(F, DT, AC);
+
+ if (IsFuncSpecEnabled)
+ Solver.addLoopInfo(F, GetLI(F));
// Determine if we can track the function's return values. If so, add the
// function to the solver's set of return-tracked functions.
@@ -222,10 +228,9 @@ static bool runIPSCCP(
BB, InsertedValues, NumInstRemoved, NumInstReplaced);
}
- DomTreeUpdater DTU = IsFuncSpecEnabled && Specializer.isClonedFunction(&F)
- ? DomTreeUpdater(DomTreeUpdater::UpdateStrategy::Lazy)
- : Solver.getDTU(F);
-
+ DominatorTree *DT = FAM->getCachedResult<DominatorTreeAnalysis>(F);
+ PostDominatorTree *PDT = FAM->getCachedResult<PostDominatorTreeAnalysis>(F);
+ DomTreeUpdater DTU(DT, PDT, DomTreeUpdater::UpdateStrategy::Lazy);
// Change dead blocks to unreachable. We do it after replacing constants
// in all executable blocks, because changeToUnreachable may remove PHI
// nodes in executable blocks we found values for. The function's entry
@@ -387,15 +392,14 @@ PreservedAnalyses IPSCCPPass::run(Module &M, ModuleAnalysisManager &AM) {
auto GetAC = [&FAM](Function &F) -> AssumptionCache & {
return FAM.getResult<AssumptionAnalysis>(F);
};
- auto getAnalysis = [&FAM, this](Function &F) -> AnalysisResultsForFn {
- DominatorTree &DT = FAM.getResult<DominatorTreeAnalysis>(F);
- return {
- std::make_unique<PredicateInfo>(F, DT, FAM.getResult<AssumptionAnalysis>(F)),
- &DT, FAM.getCachedResult<PostDominatorTreeAnalysis>(F),
- isFuncSpecEnabled() ? &FAM.getResult<LoopAnalysis>(F) : nullptr };
+ auto GetDT = [&FAM](Function &F) -> DominatorTree & {
+ return FAM.getResult<DominatorTreeAnalysis>(F);
+ };
+ auto GetLI = [&FAM](Function &F) -> LoopInfo & {
+ return FAM.getResult<LoopAnalysis>(F);
};
- if (!runIPSCCP(M, DL, &FAM, GetTLI, GetTTI, GetAC, getAnalysis,
+ if (!runIPSCCP(M, DL, &FAM, GetTLI, GetTTI, GetAC, GetDT, GetLI,
isFuncSpecEnabled()))
return PreservedAnalyses::all();
diff --git a/llvm/lib/Transforms/Utils/SCCPSolver.cpp b/llvm/lib/Transforms/Utils/SCCPSolver.cpp
index 881c3cc7b56f6..1a00bc7378c88 100644
--- a/llvm/lib/Transforms/Utils/SCCPSolver.cpp
+++ b/llvm/lib/Transforms/Utils/SCCPSolver.cpp
@@ -386,7 +386,9 @@ class SCCPInstVisitor : public InstVisitor<SCCPInstVisitor> {
using Edge = std::pair<BasicBlock *, BasicBlock *>;
DenseSet<Edge> KnownFeasibleEdges;
- DenseMap<Function *, AnalysisResultsForFn> AnalysisResults;
+ DenseMap<Function *, LoopInfo *> FnLoopInfo;
+ DenseMap<Function *, std::unique_ptr<PredicateInfo>> FnPredicateInfo;
+
DenseMap<Value *, SmallPtrSet<User *, 2>> AdditionalUsers;
LLVMContext &Ctx;
@@ -649,8 +651,12 @@ class SCCPInstVisitor : public InstVisitor<SCCPInstVisitor> {
void visitInstruction(Instruction &I);
public:
- void addAnalysis(Function &F, AnalysisResultsForFn A) {
- AnalysisResults.insert({&F, std::move(A)});
+ void addLoopInfo(Function &F, LoopInfo &LI) {
+ FnLoopInfo.insert({&F, &LI});
+ }
+
+ void addPredicateInfo(Function &F, DominatorTree &DT, AssumptionCache &AC) {
+ FnPredicateInfo.insert({&F, std::make_unique<PredicateInfo>(F, DT, AC)});
}
void visitCallInst(CallInst &I) { visitCallBase(I); }
@@ -658,23 +664,17 @@ class SCCPInstVisitor : public InstVisitor<SCCPInstVisitor> {
bool markBlockExecutable(BasicBlock *BB);
const PredicateBase *getPredicateInfoFor(Instruction *I) {
- auto A = AnalysisResults.find(I->getParent()->getParent());
- if (A == AnalysisResults.end())
+ auto It = FnPredicateInfo.find(I->getParent()->getParent());
+ if (It == FnPredicateInfo.end())
return nullptr;
- return A->second.PredInfo->getPredicateInfoFor(I);
+ return It->second->getPredicateInfoFor(I);
}
const LoopInfo &getLoopInfo(Function &F) {
- auto A = AnalysisResults.find(&F);
- assert(A != AnalysisResults.end() && A->second.LI &&
+ auto It = FnLoopInfo.find(&F);
+ assert(It != FnLoopInfo.end() && It->second &&
"Need LoopInfo analysis results for function.");
- return *A->second.LI;
- }
-
- DomTreeUpdater getDTU(Function &F) {
- auto A = AnalysisResults.find(&F);
- assert(A != AnalysisResults.end() && "Need analysis results for function.");
- return {A->second.DT, A->second.PDT, DomTreeUpdater::UpdateStrategy::Lazy};
+ return *It->second;
}
SCCPInstVisitor(const DataLayout &DL,
@@ -1950,8 +1950,13 @@ SCCPSolver::SCCPSolver(
SCCPSolver::~SCCPSolver() = default;
-void SCCPSolver::addAnalysis(Function &F, AnalysisResultsForFn A) {
- return Visitor->addAnalysis(F, std::move(A));
+void SCCPSolver::addLoopInfo(Function &F, LoopInfo &LI) {
+ Visitor->addLoopInfo(F, LI);
+}
+
+void SCCPSolver::addPredicateInfo(Function &F, DominatorTree &DT,
+ AssumptionCache &AC) {
+ Visitor->addPredicateInfo(F, DT, AC);
}
bool SCCPSolver::markBlockExecutable(BasicBlock *BB) {
@@ -1966,8 +1971,6 @@ const LoopInfo &SCCPSolver::getLoopInfo(Function &F) {
return Visitor->getLoopInfo(F);
}
-DomTreeUpdater SCCPSolver::getDTU(Function &F) { return Visitor->getDTU(F); }
-
void SCCPSolver::trackValueOfGlobalVariable(GlobalVariable *GV) {
Visitor->trackValueOfGlobalVariable(GV);
}
diff --git a/llvm/test/Transforms/SCCP/ipsccp-preserve-pdt.ll b/llvm/test/Transforms/SCCP/ipsccp-preserve-pdt.ll
new file mode 100644
index 0000000000000..5fec4151f69ba
--- /dev/null
+++ b/llvm/test/Transforms/SCCP/ipsccp-preserve-pdt.ll
@@ -0,0 +1,82 @@
+; RUN: opt -passes="ipsccp<func-spec>,print<postdomtree>" -force-specialization -funcspec-max-iters=2 -funcspec-max-clones=1 -funcspec-for-literal-constant=true -S < %s 2>&1 | FileCheck %s
+
+; REQUIRES: asserts
+
+; This test case is trying to validate that the postdomtree is preserved
+; correctly by the ipsccp pass. A tricky bug was introduced in commit
+; 1b1232047e83b69561 when PDT would be feched using getCachedAnalysis in order
+; to setup a DomTreeUpdater (to update the PDT during transformation in order
+; to preserve the analysis). But given that commit the PDT could end up being
+; required and calculated via BlockFrequency analysis. So the problem was that
+; when setting up the DomTreeUpdater we used a nullptr in case PDT wasn't
+; cached at the begininng of IPSCCP, to indicate that no updates where needed
+; for PDT. But then the PDT was calculated, given the input IR, and preserved
+; using the non-updated state (as the DTU wasn't configured for updating the
+; PDT).
+
+; CHECK-NOT: <badref>
+; CHECK: Inorder PostDominator Tree: DFSNumbers invalid: 0 slow queries.
+; CHECK-NEXT: [1] <<exit node>> {4294967295,4294967295} [0]
+; CHECK-NEXT: [2] %for.body {4294967295,4294967295} [1]
+; CHECK-NEXT: [2] %if.end4 {4294967295,4294967295} [1]
+; CHECK-NEXT: [3] %entry {4294967295,4294967295} [2]
+; CHECK-NEXT: [2] %for.cond34 {4294967295,4294967295} [1]
+; CHECK-NEXT: [3] %for.cond16 {4294967295,4294967295} [2]
+; CHECK-NEXT: Roots: %for.body %for.cond34
+; CHECK-NEXT: PostDominatorTree for function: bar
+; CHECK-NOT: <badref>
+
+declare hidden i1 @compare(ptr) align 2
+declare hidden { i8, ptr } @getType(ptr) align 2
+
+define internal void @foo(ptr %TLI, ptr %DL, ptr %Ty, ptr %ValueVTs, ptr %Offsets, i64 %StartingOffset) {
+entry:
+ %VT = alloca i64, align 8
+ br i1 false, label %if.then, label %if.end4
+
+if.then: ; preds = %entry
+ ret void
+
+if.end4: ; preds = %entry
+ %cmp = call zeroext i1 @compare(ptr undef)
+ br i1 %cmp, label %for.body, label %for.cond16
+
+for.body: ; preds = %if.end4
+ %add13 = add i64 %StartingOffset, undef
+ call void @foo(ptr %TLI, ptr %DL, ptr undef, ptr %ValueVTs, ptr %Offsets, i64 %add13)
+ unreachable
+
+for.cond16: ; preds = %for.cond34, %if.end4
+ %call27 = call { i8, ptr } @getType(ptr %VT)
+ br label %for.cond34
+
+for.cond34: ; preds = %for.body37, %for.cond16
+ br i1 undef, label %for.body37, label %for.cond16
+
+for.body37: ; preds = %for.cond34
+ %tobool39 = icmp ne ptr %Offsets, null
+ br label %for.cond34
+}
+
+define hidden { ptr, i32 } @bar(ptr %this) {
+entry:
+ %Offsets = alloca i64, align 8
+ %cmp26 = call zeroext i1 @compare(ptr undef)
+ br i1 %cmp26, label %for.body28, label %for.cond.cleanup27
+
+for.cond.cleanup27: ; preds = %entry
+ ret { ptr, i32 } undef
+
+for.body28: ; preds = %entry
+ %call33 = call zeroext i1 @compare(ptr undef)
+ br i1 %call33, label %if.then34, label %if.end106
+
+if.then34: ; preds = %for.body28
+ call void @foo(ptr %this, ptr undef, ptr undef, ptr undef, ptr null, i64 0)
+ unreachable
+
+if.end106: ; preds = %for.body28
+ call void @foo(ptr %this, ptr undef, ptr undef, ptr undef, ptr %Offsets, i64 0)
+ unreachable
+}
+
More information about the llvm-commits
mailing list