Skip to content

Commit 923ff55

Browse files
committed
[NewPM] Fix a nasty bug with analysis invalidation in the new PM.
The issue here is that we actually allow CGSCC passes to mutate IR (and therefore invalidate analyses) outside of the current SCC. At a minimum, we need to support mutating parent and ancestor SCCs to support the ArgumentPromotion pass which rewrites all calls to a function. However, the analysis invalidation infrastructure is heavily based around not needing to invalidate the same IR-unit at multiple levels. With Loop passes for example, they don't invalidate other Loops. So we need to customize how we handle CGSCC invalidation. Doing this without gratuitously re-running analyses is even harder. I've avoided most of these by using an out-of-band preserved set to accumulate the cross-SCC invalidation, but it still isn't perfect in the case of re-visiting the same SCC repeatedly *but* it coming off the worklist. Unclear how important this use case really is, but I wanted to call it out. Another wrinkle is that in order for this to successfully propagate to function analyses, we have to make sure we have a proxy from the SCC to the Function level. That requires pre-creating the necessary proxy. The motivating test case now works cleanly and is added for ArgumentPromotion. Thanks for the review from Philip and Wei! Differential Revision: https://reviews.llvm.org/D59869 llvm-svn: 357137
1 parent 8ff4585 commit 923ff55

File tree

8 files changed

+298
-184
lines changed

8 files changed

+298
-184
lines changed

llvm/include/llvm/Analysis/CGSCCPassManager.h

Lines changed: 220 additions & 169 deletions
Large diffs are not rendered by default.

llvm/lib/Analysis/CGSCCPassManager.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,12 @@ PassManager<LazyCallGraph::SCC, CGSCCAnalysisManager, LazyCallGraph &,
110110
// ...getContext().yield();
111111
}
112112

113+
// Before we mark all of *this* SCC's analyses as preserved below, intersect
114+
// this with the cross-SCC preserved analysis set. This is used to allow
115+
// CGSCC passes to mutate ancestor SCCs and still trigger proper invalidation
116+
// for them.
117+
UR.CrossSCCPA.intersect(PA);
118+
113119
// Invalidation was handled after each pass in the above loop for the current
114120
// SCC. Therefore, the remaining analysis results in the AnalysisManager are
115121
// preserved. We mark this with a set so that we don't need to inspect each

llvm/test/Other/new-pass-manager.ll

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@
2424
; CHECK-CGSCC-PASS-NEXT: Running analysis: InnerAnalysisManagerProxy<{{.*(FunctionAnalysisManager|AnalysisManager<.*Function.*>).*}},{{.*}}Module>
2525
; CHECK-CGSCC-PASS-NEXT: Running analysis: LazyCallGraphAnalysis
2626
; CHECK-CGSCC-PASS-NEXT: Running analysis: TargetLibraryAnalysis
27+
; CHECK-CGSCC-PASS-NEXT: Running analysis: FunctionAnalysisManagerCGSCCProxy
2728
; CHECK-CGSCC-PASS-NEXT: Running analysis: PassInstrumentationAnalysis
29+
; CHECK-CGSCC-PASS-NEXT: Running analysis: OuterAnalysisManagerProxy<{{.*}}LazyCallGraph::SCC{{.*}}>
2830
; CHECK-CGSCC-PASS-NEXT: Starting CGSCC pass manager run
2931
; CHECK-CGSCC-PASS-NEXT: Running pass: NoOpCGSCCPass
3032
; CHECK-CGSCC-PASS-NEXT: Finished CGSCC pass manager run
@@ -410,7 +412,9 @@
410412
; CHECK-REPEAT-CGSCC-PASS-NEXT: Running analysis: InnerAnalysisManagerProxy<{{.*(FunctionAnalysisManager|AnalysisManager<.*Function.*>).*}},{{.*}}Module>
411413
; CHECK-REPEAT-CGSCC-PASS-NEXT: Running analysis: LazyCallGraphAnalysis
412414
; CHECK-REPEAT-CGSCC-PASS-NEXT: Running analysis: TargetLibraryAnalysis
415+
; CHECK-REPEAT-CGSCC-PASS-NEXT: Running analysis: FunctionAnalysisManagerCGSCCProxy
413416
; CHECK-REPEAT-CGSCC-PASS-NEXT: Running analysis: PassInstrumentationAnalysis
417+
; CHECK-REPEAT-CGSCC-PASS-NEXT: Running analysis: OuterAnalysisManagerProxy<{{.*}}LazyCallGraph::SCC{{.*}}>
414418
; CHECK-REPEAT-CGSCC-PASS-NEXT: Starting CGSCC pass manager run
415419
; CHECK-REPEAT-CGSCC-PASS-NEXT: Running pass: RepeatedPass
416420
; CHECK-REPEAT-CGSCC-PASS-NEXT: Starting CGSCC pass manager run

llvm/test/Other/new-pm-defaults.ll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,11 +117,11 @@
117117
; CHECK-O-NEXT: Running pass: ModuleToPostOrderCGSCCPassAdaptor<{{.*}}LazyCallGraph{{.*}}>
118118
; CHECK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
119119
; CHECK-O-NEXT: Running analysis: LazyCallGraphAnalysis
120+
; CHECK-O-NEXT: Running analysis: FunctionAnalysisManagerCGSCCProxy
120121
; CHECK-O-NEXT: Running analysis: PassInstrumentationAnalysis
122+
; CHECK-O-NEXT: Running analysis: OuterAnalysisManagerProxy<{{.*}}LazyCallGraph::SCC{{.*}}>
121123
; CHECK-O-NEXT: Starting CGSCC pass manager run.
122124
; CHECK-O-NEXT: Running pass: InlinerPass
123-
; CHECK-O-NEXT: Running analysis: OuterAnalysisManagerProxy<{{.*}}LazyCallGraph{{.*}}>
124-
; CHECK-O-NEXT: Running analysis: FunctionAnalysisManagerCGSCCProxy
125125
; CHECK-O-NEXT: Running pass: PostOrderFunctionAttrsPass
126126
; CHECK-O3-NEXT: Running pass: ArgumentPromotionPass
127127
; CHECK-O-NEXT: Running pass: CGSCCToFunctionPassAdaptor<{{.*}}PassManager{{.*}}>

llvm/test/Other/new-pm-thinlto-defaults.ll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,11 +98,11 @@
9898
; CHECK-O-NEXT: Running pass: ModuleToPostOrderCGSCCPassAdaptor<{{.*}}LazyCallGraph{{.*}}>
9999
; CHECK-O-NEXT: Running analysis: InnerAnalysisManagerProxy
100100
; CHECK-O-NEXT: Running analysis: LazyCallGraphAnalysis
101+
; CHECK-O-NEXT: Running analysis: FunctionAnalysisManagerCGSCCProxy
101102
; CHECK-O-NEXT: Running analysis: PassInstrumentationAnalysis
103+
; CHECK-O-NEXT: Running analysis: OuterAnalysisManagerProxy<{{.*}}LazyCallGraph::SCC{{.*}}>
102104
; CHECK-O-NEXT: Starting CGSCC pass manager run.
103105
; CHECK-O-NEXT: Running pass: InlinerPass
104-
; CHECK-O-NEXT: Running analysis: OuterAnalysisManagerProxy<{{.*}}LazyCallGraph{{.*}}>
105-
; CHECK-O-NEXT: Running analysis: FunctionAnalysisManagerCGSCCProxy
106106
; CHECK-O-NEXT: Running pass: PostOrderFunctionAttrsPass
107107
; CHECK-O3-NEXT: Running pass: ArgumentPromotionPass
108108
; CHECK-O-NEXT: Running pass: CGSCCToFunctionPassAdaptor<{{.*}}PassManager{{.*}}>
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
; Check that when argument promotion changes a function in some parent node of
2+
; the call graph, any analyses that happened to be cached for that function are
3+
; actually invalidated. We are using `demanded-bits` here because when printed
4+
; it will end up caching a value for every instruction, making it easy to
5+
; detect the instruction-level changes that will fail here. With improper
6+
; invalidation this will crash in the second printer as it tries to reuse
7+
; now-invalid demanded bits.
8+
;
9+
; RUN: opt < %s -passes='function(print<demanded-bits>),cgscc(argpromotion,function(print<demanded-bits>))' -S | FileCheck %s
10+
11+
@G = constant i32 0
12+
13+
define internal i32 @a(i32* %x) {
14+
; CHECK-LABEL: define internal i32 @a(
15+
; CHECK-SAME: i32 %[[V:.*]]) {
16+
; CHECK-NEXT: entry:
17+
; CHECK-NEXT: ret i32 %[[V]]
18+
; CHECK-NEXT: }
19+
entry:
20+
%v = load i32, i32* %x
21+
ret i32 %v
22+
}
23+
24+
define i32 @b() {
25+
; CHECK-LABEL: define i32 @b()
26+
; CHECK-NEXT: entry:
27+
; CHECK-NEXT: %[[L:.*]] = load i32, i32* @G
28+
; CHECK-NEXT: %[[V:.*]] = call i32 @a(i32 %[[L]])
29+
; CHECK-NEXT: ret i32 %[[V]]
30+
; CHECK-NEXT: }
31+
entry:
32+
%v = call i32 @a(i32* @G)
33+
ret i32 %v
34+
}
35+
36+
define i32 @c() {
37+
; CHECK-LABEL: define i32 @c()
38+
; CHECK-NEXT: entry:
39+
; CHECK-NEXT: %[[L:.*]] = load i32, i32* @G
40+
; CHECK-NEXT: %[[V1:.*]] = call i32 @a(i32 %[[L]])
41+
; CHECK-NEXT: %[[V2:.*]] = call i32 @b()
42+
; CHECK-NEXT: %[[RESULT:.*]] = add i32 %[[V1]], %[[V2]]
43+
; CHECK-NEXT: ret i32 %[[RESULT]]
44+
; CHECK-NEXT: }
45+
entry:
46+
%v1 = call i32 @a(i32* @G)
47+
%v2 = call i32 @b()
48+
%result = add i32 %v1, %v2
49+
ret i32 %result
50+
}

llvm/test/Transforms/Inline/cgscc-incremental-invalidate.ll

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
;
99
; CHECK-LABEL: Starting llvm::Module pass manager run.
1010
; CHECK: Running pass: InlinerPass on (test1_f, test1_g, test1_h)
11-
; CHECK: Running analysis: FunctionAnalysisManagerCGSCCProxy on (test1_f, test1_g, test1_h)
1211
; CHECK: Running analysis: DominatorTreeAnalysis on test1_f
1312
; CHECK: Running analysis: DominatorTreeAnalysis on test1_g
1413
; CHECK: Invalidating all non-preserved analyses for: (test1_f)

llvm/unittests/Analysis/CGSCCPassManagerTest.cpp

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1255,26 +1255,30 @@ TEST_F(CGSCCPassManagerTest, TestAnalysisInvalidationCGSCCUpdate) {
12551255
MPM.run(*M, MAM);
12561256

12571257
// We run over four SCCs the first time. But then we split an SCC into three.
1258-
// And then we merge those three back into one.
1259-
EXPECT_EQ(4 + 3 + 1, SCCAnalysisRuns);
1258+
// And then we merge those three back into one. However, this also
1259+
// invalidates all three SCCs further down in the PO walk.
1260+
EXPECT_EQ(4 + 3 + 1 + 3, SCCAnalysisRuns);
12601261
// The module analysis pass should be run three times.
12611262
EXPECT_EQ(3, ModuleAnalysisRuns);
12621263
// We run over four SCCs the first time. Then over the two new ones. Then the
12631264
// entire module is invalidated causing a full run over all seven. Then we
1264-
// fold three SCCs back to one, and then run over the whole module again.
1265-
EXPECT_EQ(4 + 2 + 7 + 1 + 4, IndirectSCCAnalysisRuns);
1266-
EXPECT_EQ(4 + 2 + 7 + 1 + 4, DoublyIndirectSCCAnalysisRuns);
1265+
// fold three SCCs back to one, re-compute for it and the two SCCs above it
1266+
// in the graph, and then run over the whole module again.
1267+
EXPECT_EQ(4 + 2 + 7 + 1 + 3 + 4, IndirectSCCAnalysisRuns);
1268+
EXPECT_EQ(4 + 2 + 7 + 1 + 3 + 4, DoublyIndirectSCCAnalysisRuns);
12671269

12681270
// First we run over all six functions. Then we re-run it over three when we
12691271
// split their SCCs. Then we re-run over the whole module. Then we re-run
1270-
// over three functions merged back into a single SCC, and then over the
1271-
// whole module again.
1272-
EXPECT_EQ(6 + 3 + 6 + 3 + 6, FunctionAnalysisRuns);
1272+
// over three functions merged back into a single SCC, then those three
1273+
// functions again, the two functions in SCCs above it in the graph, and then
1274+
// over the whole module again.
1275+
EXPECT_EQ(6 + 3 + 6 + 3 + 3 + 2 + 6, FunctionAnalysisRuns);
12731276

12741277
// Re run the function analysis over the entire module, and then re-run it
12751278
// over the `(h3, h1, h2)` SCC due to invalidation. Then we re-run it over
12761279
// the entire module, then the three functions merged back into a single SCC,
1277-
// and then over the whole module.
1278-
EXPECT_EQ(6 + 3 + 6 + 3 + 6, IndirectFunctionAnalysisRuns);
1280+
// those three functions again, then the two functions in SCCs above it in
1281+
// the graph, and then over the whole module.
1282+
EXPECT_EQ(6 + 3 + 6 + 3 + 3 + 2 + 6, IndirectFunctionAnalysisRuns);
12791283
}
12801284
}

0 commit comments

Comments
 (0)