[PATCH] D28292: [PM] Rewrite the loop pass manager to use a worklist and augmented run arguments much like the CGSCC pass manager.
Chandler Carruth via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Jan 7 00:59:34 PST 2017
chandlerc added a comment.
Thanks so much for the review! Patch updated and responses below.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:95
+
+/// A partial specialization of the require analysis template pass.
+template <typename AnalysisT>
----------------
jlebar wrote:
> Can we add a bit of verbiage about what we're trying to accomplish here? It's not obvious to me.
Let me know if this isn't enough.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:126
+public:
+ explicit Result(LoopAnalysisManager &InnerAM, LoopInfo &LI)
+ : InnerAM(&InnerAM), LI(&LI) {}
----------------
jlebar wrote:
> Nit, Do you really want explicit on this two-arg constructor?
Well, I left it there by accident. But yea, I probably don't want to allow {}s to calls this constructor without a type name. Sadly, I want most constructors to be explicit in this day and age... =[ But its less important for multiple argument constructors. Happy to remove or leave as you see fit.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:169
+private:
+ LoopAnalysisManager *InnerAM;
+ LoopInfo *LI;
----------------
jlebar wrote:
> Here's that InnerAM again. :)
>
> I think what I'm supposed to get from this name is that this is the AM corresponding to the inner AM in the outer-to-inner proxy that is LoopAnalysisManagerFunctionProxy. Is that right?
>
> Because "LoopAnalysisManagerFunctionProxy" does not contain the words "inner" or "outer", understanding "InnerAM" in this way requires a lot of effort. It's also not clear to me what I gain from understanding it in this way? I already know it's the loop AM from its type.
>
> Suggest LoopAM, or just AM.
So the first thing to realize is that I'm specifically avoiding AM because that's this is *also* a member of the containing analysis class, and that class has a method that gets an analysis manager argument typically spelled 'AM', and this is importantly *not* the same...
Sadly, changing this to LoopAM ends up confusing as well because we don't specialize the Analysis class, and so its member continues to be spelled InnerAM. And we even see this, because we do specialize a *method* of the Analysis, specifically where we pass that InnerAM into the result. I somewhat like the names matching... But if necessary, I guess they could be mismatched... Any other ideas?
================
Comment at: include/llvm/Analysis/LoopPassManager.h:197
+/// The worklist is a LIFO data structure and we want the observed order as it
+/// is processed to be postorder so we append in append in *reverse* postorder.
+///
----------------
jlebar wrote:
> jlebar wrote:
> > , so
> Perhaps simplify "the observed order as it is processed" and put our goal first:
>
> > We want to process loops in postorder, but the worklist is a LIFO data structure, so we append to it in *reverse* postorder.
OMG, yes, thank you, so much better. Here and elsewhere, I just want to say thank you for helping me wrap the text of these comments back around to sanity. Half the time, the comments evolve over *many* rounds of edits and time, and end up with really convoluted wording. And at that point i've been staring at it far too long. I appreciate your patience rewording this stuff.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:205
+ // We use an internal worklist to build up the preorder traversal without
+ // recursion.
+ SmallVector<Loop *, 4> PreOrderLoops, PreOrderWorklist;
----------------
jlebar wrote:
> Not sure this comment is necessary.
I'm trying to explain why we have a second worklist?
================
Comment at: include/llvm/Analysis/LoopPassManager.h:253
+/// provided as an argument to each loop pass.
+class LPMUpdateResult {
+public:
----------------
jlebar wrote:
> This is kind of a weird name -- at least as described above, it doesn't sound like a "result". LPMUpdater?
So, for both this and the analysis results, I left in somewhat intentionally bad names because i wanted to get help naming them.
I generally like ...Updater here, and ...AnalysisResults above.
My biggest problem is -- what prefix? LPM is actually kinda misleading as they're much more about loop passes and not about loop pass management. I could go with LP, but it seems a pretty lame prefix. I could go with LoopPass to at least avoid the inscrutability of LP at the cost of longer names.
Halp...
================
Comment at: include/llvm/Analysis/LoopPassManager.h:261
+ /// directly
+ /// as it may no longer be in a valid state.
+ bool skipCurrentLoop() const { return SkipCurrentLoop; }
----------------
jlebar wrote:
> jlebar wrote:
> > jlebar wrote:
> > > reflow comment
> > , as
> Do you mean
>
> > If this returns true, the loop object may have been deleted, so passes should take care not to touch the object.
>
> ?
Yes, yes I do.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:295
+
+ // Also skip further processing of the current loop, it will be revisited
+ // after all of its newly added children are accounted for.
----------------
jlebar wrote:
> s/,/ --/ or s/, i/: I/
I have such a complicated relationship with commas....
================
Comment at: include/llvm/Analysis/LoopPassManager.h:306
+ /// loop pass manager.
+ void addSiblingLoops(ArrayRef<Loop *> NewSibLoops) {
+ internal::appendLoopsToWorklist(NewSibLoops, Worklist);
----------------
jlebar wrote:
> Should we assert that the loops are actually siblings of the current loop? Similarly in addChildLoops, should we check that the loops are actually children?
Yes. I meant to add those asserts and then didn't. Thanks for catching that.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:307
+ void addSiblingLoops(ArrayRef<Loop *> NewSibLoops) {
+ internal::appendLoopsToWorklist(NewSibLoops, Worklist);
+
----------------
jlebar wrote:
> Do you want to have an assertion in appendLoopsToWorklist (or here if you can't put it there) that none of the loops are nested within one another, to catch the invariant described above?
The assert seems easier to do here while we're checking that they're siblings.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:317
+ // Fixed parameters for a given walk over a function's loop nests.
+ SmallPriorityWorklist<Loop *, 4> &Worklist;
+ LoopAnalysisManager &LAM;
----------------
jlebar wrote:
> This changes if you call addSiblingLoops or addChildLoops, so it doesn't seem so fixed. Maybe we can just say what it is instead?
What it refers to doesn't change is what I meant.... But sure, better comments added instead.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:370
+ // Setup the update result struct to let passes control our walk of the
+ // loops.
+ LPMUpdateResult UR(Worklist, LAM);
----------------
jlebar wrote:
> Update this comment if we change the name of LPMUpdateResult.
I can make the comment less redundant and avoid the need to update...
================
Comment at: lib/Analysis/LoopPassManager.cpp:32
+/// Explicitly specialize the pass manager run method to handle call graph
+/// updates.
template <>
----------------
jlebar wrote:
> It's not call graph updates we care about, right? It's the addition / removal of loops?
I WOULD NEVER CUT AND PASTE CODE BLINDLY FROM ONE FILE TO ANOTHER AND FAIL TO UPDATE ALL THE COMMENTS!
Except of course for all of this code where I did exactly that. Sorry. ;]
================
Comment at: lib/Analysis/LoopPassManager.cpp:41
+
+ if (DebugLogging)
+ dbgs() << "Starting Loop pass manager run.\n";
----------------
jlebar wrote:
> For my information, why are we using a boolean rather than the DEBUG infrastructure? For one thing, DEBUG compiled away in the builds we release, whereas llvm::dbgs() does not, right?
Because I haven't gone back and undone this. :: sigh ::
I do have an idea for how to fix this to be a little less painful. We want to keep a small amount of this stuff even in no-asserts builds, but *WAY* less than currently, so most of this goes into the lovely DEBUG macro. ANyways, planned for a future patch that does this across all the PMs. I can expedite that if useful.
================
Comment at: lib/Analysis/LoopPassManager.cpp:72
+ // Invalidation was handled after each pass in the above loop for the current
+ // SCC. Therefore, the remaining analysis results in the AnalysisManager are
+ // preserved. We mark this with a set so that we don't need to inspect each
----------------
jlebar wrote:
> What do SCCs have to do with this? (Stale comment?)
Yep.
However, this code doesn't work as written it occurs to me. I've left a FIXME for now, I need to think about how to handle this.
================
Comment at: lib/Analysis/LoopPassManager.cpp:135
+ // like to preserve the integrity of other cached entries in the analysis
+ // manager. This also should catch any attempt to use the analysis manager
+ // via this proxy prior to it being rebuilt.
----------------
jlebar wrote:
> Can you rephrase the "Because" sentence?
Tried rewriting this whole thing. Lemme know if this is better.
================
Comment at: lib/Analysis/LoopPassManager.cpp:141
+
+ // New return true to indicate this *is* invalid and a fresh proxy result
+ // needs to be built.
----------------
jlebar wrote:
> s/New/Now/? Otherwise I don't know what you mean.
Also added more detail.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:235
" ret void\n"
- "}\n")) {}
-};
+ "}\n")),
+ LAM(true), FAM(true), MAM(true) {
----------------
jlebar wrote:
> 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).
Yea, I just haven't gotten around to cleaning all of these up... Will try to actually do that, but would rather not in this patch.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:236
+ "}\n")),
+ LAM(true), FAM(true), MAM(true) {
+ // Register an analysis from the mock handle.
----------------
jlebar wrote:
> 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
Yep. But what would be even more nice is to actually accept a more useful argument like the raw_ostream to use. ;] Anyways, I had planned to fix this when lessenging the use of it at all and using DEBUG() more heavily.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:266
+ // once for each loop.
+ EXPECT_CALL(MLPHandle, run(HasName("loop.0.0"), _, _, _))
+ .WillOnce(Invoke(getLoopAnalysisResult));
----------------
jlebar wrote:
> 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?
The problem is that even though we don't really need to *match* on these arguments, we do want to have access to them in the Invoke'd actions.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:283
FPM.addPass(createFunctionToLoopPassAdaptor(std::move(LPM)));
+ MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
}
----------------
jlebar wrote:
> 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.
Yep, good suggestion. Let me know if my commets are helping?
================
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.
----------------
jlebar wrote:
> s/themselves//?
Tried a better word. Trying to differentiate from failing to preserve the LoopAnalysis.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:514
+
+ // The rest don't invalidate, only trigger re-runs because we clear the cache
+ // completely.
----------------
jlebar wrote:
> jlebar wrote:
> > triggers
> s/,/; it/
It's not an it though.
I think this should be "the rest don't invalidate; they only trigger ..." which then agrees?
(But I so don't trust myself here...)
================
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);
----------------
jlebar wrote:
> required?
no, this is a RequiresAnalysis pass thingy....
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:900
+ .Times(2)
+ .WillRepeatedly(Invoke(getLoopAnalysisResult));
----------------
jlebar wrote:
> Same here about chaining WillOnce, and same below.
Same what about chaining WillOnce?
================
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);
----------------
jlebar wrote:
> At this point in the file this comment is probably unnecessary.
I was being consistent and trying to help the reader that just came into this file at the bottom...
================
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:
> jlebar wrote:
> > outermost
> s/iteration/loop/?
Yes, but i dunno why I wrote this. the key thing here is that this is testing *top-level* insertion. Commented accordingly now.
================
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);
----------------
jlebar wrote:
> Same re this comment, not sure it's necessary by this point.
Same as above. I can nuke them all if yo uwant, but i'd as soon have all of them as one of them...
================
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
----------------
jlebar wrote:
> 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?
the which is an aside, you were correct to suggest a comma.
================
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
----------------
jlebar wrote:
> , and
You shoul count the commas and fine me or something....
https://reviews.llvm.org/D28292
More information about the llvm-commits
mailing list