[llvm] r292757 - [PM] Fix a really nasty bug introduced when adding PGO support to the
Chandler Carruth via llvm-commits
llvm-commits at lists.llvm.org
Sun Jan 22 02:34:01 PST 2017
Author: chandlerc
Date: Sun Jan 22 04:34:01 2017
New Revision: 292757
URL: http://llvm.org/viewvc/llvm-project?rev=292757&view=rev
Log:
[PM] Fix a really nasty bug introduced when adding PGO support to the
new PM's inliner.
The bug happens when we refine an SCC after having computed a proxy for
the FunctionAnalysisManager, and then proceed to compute fresh analyses
for functions in the *new* SCC using the manager provided by the old
SCC's proxy. *And* when we manage to mutate a function in this new SCC
in a way that invalidates those analyses. This can be... challenging to
reproduce.
I've managed to contrive a set of functions that trigger this and added
a test case, but it is a bit brittle. I've directly checked that the
passes run in the expected ways to help avoid the test just becoming
silently irrelevant.
This gets the new PM back to passing the LLVM test suite after the PGO
improvements landed.
Added:
llvm/trunk/test/Transforms/Inline/cgscc-incremental-invalidate.ll
Modified:
llvm/trunk/lib/Transforms/IPO/Inliner.cpp
llvm/trunk/test/Other/new-pm-defaults.ll
Modified: llvm/trunk/lib/Transforms/IPO/Inliner.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/Inliner.cpp?rev=292757&r1=292756&r2=292757&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/IPO/Inliner.cpp (original)
+++ llvm/trunk/lib/Transforms/IPO/Inliner.cpp Sun Jan 22 04:34:01 2017
@@ -751,9 +751,6 @@ bool LegacyInlinerBase::removeDeadFuncti
PreservedAnalyses InlinerPass::run(LazyCallGraph::SCC &InitialC,
CGSCCAnalysisManager &AM, LazyCallGraph &CG,
CGSCCUpdateResult &UR) {
- FunctionAnalysisManager &FAM =
- AM.getResult<FunctionAnalysisManagerCGSCCProxy>(InitialC, CG)
- .getManager();
const ModuleAnalysisManager &MAM =
AM.getResult<ModuleAnalysisManagerCGSCCProxy>(InitialC, CG).getManager();
bool Changed = false;
@@ -762,21 +759,6 @@ PreservedAnalyses InlinerPass::run(LazyC
Module &M = *InitialC.begin()->getFunction().getParent();
ProfileSummaryInfo *PSI = MAM.getCachedResult<ProfileSummaryAnalysis>(M);
- std::function<AssumptionCache &(Function &)> GetAssumptionCache =
- [&](Function &F) -> AssumptionCache & {
- return FAM.getResult<AssumptionAnalysis>(F);
- };
- auto GetBFI = [&](Function &F) -> BlockFrequencyInfo & {
- return FAM.getResult<BlockFrequencyAnalysis>(F);
- };
-
- auto GetInlineCost = [&](CallSite CS) {
- Function &Callee = *CS.getCalledFunction();
- auto &CalleeTTI = FAM.getResult<TargetIRAnalysis>(Callee);
- return getInlineCost(CS, Params, CalleeTTI, GetAssumptionCache, {GetBFI},
- PSI);
- };
-
// We use a worklist of nodes to process so that we can handle if the SCC
// structure changes and some nodes are no longer part of the current SCC. We
// also need to use an updatable pointer for the SCC as a consequence.
@@ -816,6 +798,31 @@ PreservedAnalyses InlinerPass::run(LazyC
DEBUG(dbgs() << "Inlining calls in: " << F.getName() << "\n");
+ // Get a FunctionAnalysisManager via a proxy for this particular node. We
+ // do this each time we visit a node as the SCC may have changed and as
+ // we're going to mutate this particular function we want to make sure the
+ // proxy is in place to forward any invalidation events. We can use the
+ // manager we get here for looking up results for functions other than this
+ // node however because those functions aren't going to be mutated by this
+ // pass.
+ FunctionAnalysisManager &FAM =
+ AM.getResult<FunctionAnalysisManagerCGSCCProxy>(*C, CG)
+ .getManager();
+ std::function<AssumptionCache &(Function &)> GetAssumptionCache =
+ [&](Function &F) -> AssumptionCache & {
+ return FAM.getResult<AssumptionAnalysis>(F);
+ };
+ auto GetBFI = [&](Function &F) -> BlockFrequencyInfo & {
+ return FAM.getResult<BlockFrequencyAnalysis>(F);
+ };
+
+ auto GetInlineCost = [&](CallSite CS) {
+ Function &Callee = *CS.getCalledFunction();
+ auto &CalleeTTI = FAM.getResult<TargetIRAnalysis>(Callee);
+ return getInlineCost(CS, Params, CalleeTTI, GetAssumptionCache, {GetBFI},
+ PSI);
+ };
+
// Get the remarks emission analysis for the caller.
auto &ORE = FAM.getResult<OptimizationRemarkEmitterAnalysis>(F);
Modified: llvm/trunk/test/Other/new-pm-defaults.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Other/new-pm-defaults.ll?rev=292757&r1=292756&r2=292757&view=diff
==============================================================================
--- llvm/trunk/test/Other/new-pm-defaults.ll (original)
+++ llvm/trunk/test/Other/new-pm-defaults.ll Sun Jan 22 04:34:01 2017
@@ -62,8 +62,8 @@
; CHECK-O-NEXT: Running analysis: LazyCallGraphAnalysis
; CHECK-O-NEXT: Starting CGSCC pass manager run.
; CHECK-O-NEXT: Running pass: InlinerPass
-; CHECK-O-NEXT: Running analysis: FunctionAnalysisManagerCGSCCProxy
; CHECK-O-NEXT: Running analysis: OuterAnalysisManagerProxy<{{.*}}LazyCallGraph{{.*}}>
+; CHECK-O-NEXT: Running analysis: FunctionAnalysisManagerCGSCCProxy
; CHECK-O-NEXT: Running analysis: OptimizationRemarkEmitterAnalysis
; CHECK-O-NEXT: Running pass: PostOrderFunctionAttrsPass
; CHECK-O-NEXT: Running pass: CGSCCToFunctionPassAdaptor<{{.*}}PassManager{{.*}}>
Added: llvm/trunk/test/Transforms/Inline/cgscc-incremental-invalidate.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/cgscc-incremental-invalidate.ll?rev=292757&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/Inline/cgscc-incremental-invalidate.ll (added)
+++ llvm/trunk/test/Transforms/Inline/cgscc-incremental-invalidate.ll Sun Jan 22 04:34:01 2017
@@ -0,0 +1,111 @@
+; Test for a subtle bug when computing analyses during inlining and mutating
+; the SCC structure. Without care, this can fail to invalidate analyses.
+;
+; RUN: opt < %s -passes='cgscc(inline,function(verify<domtree>))' -debug-pass-manager -S 2>&1 | FileCheck %s
+
+; First we check that the passes run in the way we expect. Otherwise this test
+; may stop testing anything.
+;
+; CHECK-LABEL: Starting llvm::Module pass manager run.
+; CHECK: Running pass: InlinerPass on (test1_h, test1_g, test1_f)
+; CHECK: Running analysis: FunctionAnalysisManagerCGSCCProxy on (test1_h, test1_g, test1_f)
+; CHECK: Running analysis: DominatorTreeAnalysis on test1_f
+; CHECK: Running analysis: DominatorTreeAnalysis on test1_g
+; CHECK: Invalidating all non-preserved analyses for: (test1_h, test1_g, test1_f)
+; CHECK: Invalidating all non-preserved analyses for: test1_h
+; CHECK-NOT: Invalidating anaylsis:
+; CHECK: Invalidating all non-preserved analyses for: test1_g
+; CHECK: Invalidating analysis: DominatorTreeAnalysis on test1_g
+; CHECK: Invalidating all non-preserved analyses for: test1_f
+; CHECK: Invalidating analysis: DominatorTreeAnalysis on test1_f
+; CHECK: Running analysis: DominatorTreeAnalysis on test1_h
+; CHECK: Invalidating all non-preserved analyses for: (test1_h, test1_g)
+; CHECK: Invalidating all non-preserved analyses for: test1_h
+; CHECK: Invalidating analysis: DominatorTreeAnalysis on test1_h
+
+; An external function used to control branches.
+declare i1 @flag()
+; CHECK-LABEL: declare i1 @flag()
+
+; The utility function with interesting control flow that gets inlined below to
+; perturb the dominator tree.
+define internal void @callee() {
+entry:
+ %ptr = alloca i8
+ %flag = call i1 @flag()
+ br i1 %flag, label %then, label %else
+
+then:
+ store volatile i8 42, i8* %ptr
+ br label %return
+
+else:
+ store volatile i8 -42, i8* %ptr
+ br label %return
+
+return:
+ ret void
+}
+
+; The 'test1_' prefixed functions work to carefully test that incrementally
+; reducing an SCC in the inliner cannot accidentially leave stale function
+; analysis results due to failing to invalidate them for all the functions.
+
+; We visit this function first in the inliner, and while we inline callee
+; perturbing the CFG, we don't inline anything else and the SCC structure
+; remains in tact.
+define void @test1_f() {
+; CHECK-LABEL: define void @test1_f()
+entry:
+ ; We force this edge to survive inlining.
+ call void @test1_g() noinline
+; CHECK: call void @test1_g()
+
+ ; Pull interesting CFG into this function.
+ call void @callee()
+; CHECK-NOT: call void @callee()
+
+ ret void
+; CHECK: ret void
+}
+
+; Next we visit this function and here we inline the edge to 'test1_f'
+; separating it into its own SCC. The current SCC is now just 'test1_g' and
+; 'test1_h'.
+define void @test1_g() {
+; CHECK-LABEL: define void @test1_g()
+entry:
+ ; This edge gets inlined away.
+ call void @test1_f()
+; CHECK-NOT: call void @test1_f()
+; CHECK: call void @test1_g()
+
+ ; We force this edge to survive inlining.
+ call void @test1_h() noinline
+; CHECK: call void @test1_h()
+
+ ; Pull interesting CFG into this function.
+ call void @callee()
+; CHECK-NOT: call void @callee()
+
+ ret void
+; CHECK: ret void
+}
+
+; Finally the inliner visits this last function. It can't actually break any
+; cycles here, but because we visit this function we compute fresh analyses for
+; it. These analyses are then invalidated when we inline callee disrupting the
+; CFG, and it is important that they be freed.
+define void @test1_h() {
+; CHECK-LABEL: define void @test1_h()
+entry:
+ call void @test1_g()
+; CHECK: call void @test1_g()
+
+ ; Pull interesting CFG into this function.
+ call void @callee()
+; CHECK-NOT: call void @callee()
+
+ ret void
+; CHECK: ret void
+}
More information about the llvm-commits
mailing list