[PATCH] D28292: [PM] Rewrite the loop pass manager to use a worklist and augmented run arguments much like the CGSCC pass manager.
Justin Lebar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 5 13:52:56 PST 2017
jlebar added a comment.
I <3 gmock. Strong approve; this would be really hard to write without.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:235
" ret void\n"
- "}\n")) {}
-};
+ "}\n")),
+ LAM(true), FAM(true), MAM(true) {
----------------
FWIW -- I think we talked about this elsewhere, but I forget the conclusion -- I would find a raw string far easier to read (and modify).
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:236
+ "}\n")),
+ LAM(true), FAM(true), MAM(true) {
+ // Register an analysis from the mock handle.
----------------
Nit, it would be really nice if we'd adopt the boolean arg-naming convention, `foo(/*some_arg=*/false)` -- see also the now five-year-old blog post that keeps on giving, http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:237
+ LAM(true), FAM(true), MAM(true) {
+ // Register an analysis from the mock handle.
+ LAM.registerPass([&] { return MLAHandle.getAnalysis(); });
----------------
Maybe
> Register MLAHandle's analysis.
or even
> Register our mock analysis.
? I like the latter because it says what we're doing without just repeating what's in the code.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:251
-TEST_F(LoopPassManagerTest, Basic) {
- LoopAnalysisManager LAM(true);
- int LoopAnalysisRuns = 0;
- LAM.registerPass([&] { return TestLoopAnalysis(LoopAnalysisRuns); });
-
- FunctionAnalysisManager FAM(true);
- // We need DominatorTreeAnalysis for LoopAnalysis.
- FAM.registerPass([&] { return DominatorTreeAnalysis(); });
- FAM.registerPass([&] { return LoopAnalysis(); });
- // We also allow loop passes to assume a set of other analyses and so need
- // those.
- FAM.registerPass([&] { return AAManager(); });
- FAM.registerPass([&] { return TargetLibraryAnalysis(); });
- FAM.registerPass([&] { return ScalarEvolutionAnalysis(); });
- FAM.registerPass([&] { return AssumptionAnalysis(); });
- FAM.registerPass([&] { return LoopAnalysisManagerFunctionProxy(LAM); });
- LAM.registerPass([&] { return FunctionAnalysisManagerLoopProxy(FAM); });
-
- ModuleAnalysisManager MAM(true);
- MAM.registerPass([&] { return FunctionAnalysisManagerModuleProxy(FAM); });
- FAM.registerPass([&] { return ModuleAnalysisManagerFunctionProxy(MAM); });
+ // Cross register proxies.
+ LAM.registerPass([&] { return FunctionAnalysisManagerLoopProxy(FAM); });
----------------
s/Cross register/Cross-register/
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:264
+ // First we just visit all the loops in all the functions and get the
+ // analysis result for it. This will run the analysis a total of four times,
+ // once for each loop.
----------------
> analysis results for them
or, probably better
> get their analysis results
?
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:266
+ // once for each loop.
+ EXPECT_CALL(MLPHandle, run(HasName("loop.0.0"), _, _, _))
+ .WillOnce(Invoke(getLoopAnalysisResult));
----------------
All or almost all of the expectations on run() are of the form
run(HasName("foo"), _, _, _)
It would be kind of nice to simplify this to
run(HasName("foo"))
which we could do by changing the MOCK_METHOD4 to MOCK_METHOD1, per https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#simplifying-the-interface-without-breaking-existing-code
I'm on the fence about it. WTYT?
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:268
+ .WillOnce(Invoke(getLoopAnalysisResult));
+ EXPECT_CALL(MLAHandle, run(HasName("loop.0.0"), _, _)).Times(1);
+ EXPECT_CALL(MLPHandle, run(HasName("loop.0.1"), _, _, _))
----------------
https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.md#cardinalities-how-many-times-will-it-be-called suggests Times(1) is not needed.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:283
FPM.addPass(createFunctionToLoopPassAdaptor(std::move(LPM)));
+ MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
}
----------------
I pondered over this for a good five minutes before figuring it out.
I think part of my problem was that based on the comments and the scoping, it seems that this runs some passes somehow.
It's clearer to me in the other tests because we don't have an explicit scope.
Maybe just a brief comment on this block would fix my confusion.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:374
+ .WillOnce(DoDefault());
+ // For 'g', fail to preserve anything causing the loops themselves to be
+ // cleared. We don't get an invalidation event here as the loop is gone, but
----------------
, causing
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:389
+
+ // Verify with a separate function pass run that we didn't mess upp 'f's
+ // cache. No analysis runs should be necessary here.
----------------
upp
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:414
+
+ // But a module pass that doesn't preserve the loop analyses themselves
+ // invalidates all the way down and forces recomputing.
----------------
s/themselves//?
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:514
+
+ // The rest don't invalidate, only trigger re-runs because we clear the cache
+ // completely.
----------------
triggers
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:514
+
+ // The rest don't invalidate, only trigger re-runs because we clear the cache
+ // completely.
----------------
jlebar wrote:
> triggers
s/,/; it/
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:639
+
+ // Setup our 'A' analysis to depend on our 'B' analysis. For testing purposes
+ // we just need to trigger getting the 'B' analysis results in 'A's run
----------------
"Set up" :)
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:640
+ // Setup our 'A' analysis to depend on our 'B' analysis. For testing purposes
+ // we just need to trigger getting the 'B' analysis results in 'A's run
+ // method and check if 'B' gets invalidate in 'A's invalidate method.
----------------
"just need to get"
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:641
+ // we just need to trigger getting the 'B' analysis results in 'A's run
+ // method and check if 'B' gets invalidate in 'A's invalidate method.
+ ON_CALL(MLAHandleA, run(_, _, _))
----------------
gets invalidated
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:641
+ // we just need to trigger getting the 'B' analysis results in 'A's run
+ // method and check if 'B' gets invalidate in 'A's invalidate method.
+ ON_CALL(MLAHandleA, run(_, _, _))
----------------
jlebar wrote:
> gets invalidated
s/our 'A' analysis/AnalysisA/, and likewise for AnalysisB?
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:677
+ }));
+ // It happens that 'B' is invalidated first. That shouldn't matter though and
+ // we should still call 'A"s invalidation.
----------------
, and
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:678
+ // It happens that 'B' is invalidated first. That shouldn't matter though and
+ // we should still call 'A"s invalidation.
+ EXPECT_CALL(MLAHandleB, invalidate(HasName("loop.0.0"), _, _)).Times(1);
----------------
s/"/'/
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:705
+
+ // The run over 'g' should be boring with us just computing the analyses once
+ // up front and then running loop passes and getting cached results.
----------------
, with
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:737
+
+ // Setup the loop analysis to depend on both the function and module analysis
+ // by default.
----------------
Set up
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:738
+ // Setup the loop analysis to depend on both the function and module analysis
+ // by default.
+ ON_CALL(MLAHandle, run(_, _, _))
----------------
s/by default//?
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:765
+
+ // Now invalidate the function analysis but preserving the loop analyses
+ // which should trigger immediate invalidation of the loop analyses despite
----------------
preserve
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:766
+ // Now invalidate the function analysis but preserving the loop analyses
+ // which should trigger immediate invalidation of the loop analyses despite
+ // being preserved.
----------------
New sentence, "This should trigger"
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:766
+ // Now invalidate the function analysis but preserving the loop analyses
+ // which should trigger immediate invalidation of the loop analyses despite
+ // being preserved.
----------------
jlebar wrote:
> New sentence, "This should trigger"
", despite their being preserved." or ", despite the fact that they were preserved."
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:777
+
+ // And re-running a requires pass recomputes them.
+ EXPECT_CALL(MLAHandle, run(HasName("loop.0.0"), _, _)).Times(1);
----------------
required?
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:859
+
+ // All the visit orders are deterministic so we use simple fully order
+ // expectations.
----------------
, so
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:900
+ .Times(2)
+ .WillRepeatedly(Invoke(getLoopAnalysisResult));
----------------
Same here about chaining WillOnce, and same below.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:907
- // The first LPM runs the loop analysis for all four loops, the second uses
- // cached results for everything.
- EXPECT_EQ(4, LoopAnalysisRuns);
+ // The second run over the middle loop after we've visited the new child, we
+ // add another child to check that we can repeatedly add children, and add
----------------
In the second run
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:958
+ // Now that all the expected actions are registered, run the pipeline over
+ // our module. All of our expectations are verified when the test finishes.
+ MPM.run(*M, MAM);
----------------
At this point in the file this comment is probably unnecessary.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1040
+ }));
+ // We finish processing this loop as sibling loops don't perturb the
+ // postorder walk.
----------------
, as
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1058
+ .WillOnce(Invoke(getLoopAnalysisResult));
+ // Next, on the third pass run on last inner loop we add more new siblings,
+ // more than one, and one with nested child loops. By doing this at the end
----------------
the last inner loop
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1115
+ EXPECT_CALL(MLAHandle, run(HasName("loop.0"), _, _)).Times(1);
+ // On the second pass, we add sibling loops to the outer-most iteration.
+ EXPECT_CALL(MLPHandle, run(HasName("loop.0"), _, _, _))
----------------
outermost
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1115
+ EXPECT_CALL(MLAHandle, run(HasName("loop.0"), _, _)).Times(1);
+ // On the second pass, we add sibling loops to the outer-most iteration.
+ EXPECT_CALL(MLPHandle, run(HasName("loop.0"), _, _, _))
----------------
jlebar wrote:
> outermost
s/iteration/loop/?
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1148
+ // Now that all the expected actions are registered, run the pipeline over
+ // our module. All of our expectations are verified when the test finishes.
+ MPM.run(*M, MAM);
----------------
Same re this comment, not sure it's necessary by this point.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1284
+
+ // Run the loop pipeline again. This time we delete the last loop which
+ // contains a nested loop within it, and we re-use that loop object to insert
----------------
ITYM ", which", but not sure. Do you mean you delete the last loop period, or that you delete the last loop that happens to contain another loop?
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1285
+ // Run the loop pipeline again. This time we delete the last loop which
+ // contains a nested loop within it, and we re-use that loop object to insert
+ // a new loop into the nest. This makes sure that we don't reuse cached
----------------
reuse
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1308
+ DeleteLoopBlocks(**L.begin(), Loop02BB, AR);
+ // Remove and save the child loop object to re-use later.
+ UR.markLoopAsDeleted(**L.begin());
----------------
reuse
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1319
+
+ // Now insert a new sibling loop re-using a loop pointer.
+ ParentL->addChildLoop(OldL);
----------------
, reusing
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1330
+
+ // We should visit the newly inserted sibling to the just deleted loop first
+ // here as it is still a postorder constraint prior to visiting the outer
----------------
just-deleted
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1333
+ // loop. It is important that this computes a fresh analysis result rather
+ // than using a cached result due to the same loop object acting as a key.
+ EXPECT_CALL(MLPHandle, run(HasName("loop.0.3"), _, _, _))
----------------
Can we rephrase?
> To respect our inner-to-outer traversal order, we must visit the newly-inserted sibling of the loop we just deleted before we visit the outer loop. When we do so, this must compute a fresh analysis result, even though our new loop has the same pointer value as the loop we deleted.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1345
+
+ // In the final loop pipeline run we delete every loop including the last
+ // loop of the nest. We do this again in the second pass in the pipeline and
----------------
, including
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1346
+ // In the final loop pipeline run we delete every loop including the last
+ // loop of the nest. We do this again in the second pass in the pipeline and
+ // as a consequence we never make it to three runs on any loop. We also cover
----------------
, and
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:1391
+ // Add the function pass pipeline now that it is fully built up and run it
+ // over the module's one function.
+ MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
----------------
Same wrt this comment.
https://reviews.llvm.org/D28292
More information about the llvm-commits
mailing list