[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
Sun Jan 8 11:42:30 PST 2017


jlebar accepted this revision.
jlebar added a comment.
This revision is now accepted and ready to land.

\o/



================
Comment at: include/llvm/Analysis/LoopPassManager.h:126
+public:
+  explicit Result(LoopAnalysisManager &InnerAM, LoopInfo &LI)
+      : InnerAM(&InnerAM), LI(&LI) {}
----------------
chandlerc wrote:
> 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.
> Sadly, I want most constructors to be explicit in this day and age... =[

Yeah, understood.  So long as it's intentional, that's cool.


================
Comment at: include/llvm/Analysis/LoopPassManager.h:169
+private:
+  LoopAnalysisManager *InnerAM;
+  LoopInfo *LI;
----------------
chandlerc wrote:
> 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?
> 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

Sorry if this is obvious, who exactly has an AM member variable?  Not LoopAnalysisManagerFunctionProxy == InnerAnalysisManagerFunctionProxy afaict?  The outer proxy does, but the inner one's member is named `InnerAM`, like you say.  I looked through all of PassManager.h and don't see anyone else that seems relevant with a member called `AM`.

> 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. 

OK, I'm sold on that basis.


================
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;
----------------
chandlerc wrote:
> jlebar wrote:
> > Not sure this comment is necessary.
> I'm trying to explain why we have a second worklist?
Hm, you're saying we can't use Worklist for the preorder traversal?  That seems obvious to me because our goal here is to add some stuff to Worklist, but maybe it's my ignorance that's making me think that's obvious.  You could say

> Worklist is our output param; we need a second worklist for the preorder traversal.

?


================
Comment at: include/llvm/Analysis/LoopPassManager.h:253
+/// provided as an argument to each loop pass.
+class LPMUpdateResult {
+public:
----------------
chandlerc wrote:
> 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...
LPMUpdateResult seems to be more about updating the LPM, not updating an LP.  It's the class that LPs use to update the LPM, as I read it.  So LPMUpdater or LoopPassManagerUpdater seems fine.

I agree that the analysis results one is less about the manager.  LPAnalysisResults or LoopPassAnalysisResults?  I kind of want to say that they're the analysis results you get "for free".  LPStandardAnalysisResults?

I also agree you should be consistent in whether you abbreviate "LP/M".


================
Comment at: include/llvm/Analysis/LoopPassManager.h:97
+/// forward the necessary extra parameters from a transformation's run method
+/// to the AnalysisManager's getResult.
+template <typename AnalysisT>
----------------
Ah, got it.  lgtm, but suggest `s/correctly// s/necessary//`.


================
Comment at: include/llvm/Analysis/LoopPassManager.h:431
+    // We also preserve the set of analyses queried up-front and preserved
+    // throughout the run. This avoids each individual loop pass having to mark
+    // this.
----------------
It would be cool if this set of analyses had a name, then you could say

> We also preserve the standard loop analyses [or whatever].


================
Comment at: lib/Analysis/LoopPassManager.cpp:91
+  // to visit this in postorder, but because this is a tree structure we can
+  // build a preorder sequence and walk it in reverse to do this.
+  SmallVector<Loop *, 4> PreOrderLoops, PreOrderWorklist;
----------------
Suggest

> we can do this by building a preorder sequence and walking it in reverse


================
Comment at: lib/Analysis/LoopPassManager.cpp:112
+  // to clear all the keys coming from that analysis. We also completely blow
+  // away the loop analyses if any of the "bundled" analyses provided by the
+  // loop pass manager go away so that loop analyses can freely use these
----------------
here too it would be good to have a standard name for these analyses.


================
Comment at: lib/Analysis/LoopPassManager.cpp:133
+
+    // We also need to null out the inner AM so that when the result object
+    // gets destroyed as invalid we don't try to clear the inner AM again. At
----------------
s/the result object/this object/?


================
Comment at: lib/Analysis/LoopPassManager.cpp:41
+
+  if (DebugLogging)
+    dbgs() << "Starting Loop pass manager run.\n";
----------------
chandlerc wrote:
> 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.
> I can expedite that if useful.

No need.


================
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.
----------------
chandlerc wrote:
> jlebar wrote:
> > Can you rephrase the "Because" sentence?
> Tried rewriting this whole thing. Lemme know if this is better.
It's a lot better, but now that I understand what you're saying, I think this should be combined into the comment above.  AIUI the loop above is taking the place of a call to InnerAM->clear() in the destructor, so the two comments are related.

> Walk the loops for this function and clear the results associated with this function.  Then set InnerAM to null so that we don't call InnerAM->clear() (which nukes everything from orbit) when this object is destroyed as invalid.
>
> Note that although the LoopInfo may be stale at this point, the loop objects themselves remain the only viable keys that could be in the analysis manager's cache, so we can just walk the keys and forcibly clear those results.  The order doesn't matter here as this will directly destroy the results without calling methods on them.
>
> FIXME ...


================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:235
                            "  ret void\n"
-                           "}\n")) {}
-};
+                           "}\n")),
+        LAM(true), FAM(true), MAM(true) {
----------------
chandlerc wrote:
> 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.
np, sgtm.


================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:236
+                           "}\n")),
+        LAM(true), FAM(true), MAM(true) {
+    // Register an analysis from the mock handle.
----------------
chandlerc wrote:
> 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.
sgtm


================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:266
+  // once for each loop.
+  EXPECT_CALL(MLPHandle, run(HasName("loop.0.0"), _, _, _))
+      .WillOnce(Invoke(getLoopAnalysisResult));
----------------
chandlerc wrote:
> 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.
Ah, scratch that plan then.


================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:283
     FPM.addPass(createFunctionToLoopPassAdaptor(std::move(LPM)));
+    MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
   }
----------------
chandlerc wrote:
> 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?
Yes, looks good.


================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:900
+      .Times(2)
+      .WillRepeatedly(Invoke(getLoopAnalysisResult));
 
----------------
chandlerc wrote:
> jlebar wrote:
> > Same here about chaining WillOnce, and same below.
> Same what about chaining WillOnce?
Sorry, that was related to a comment I deleted.


================
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);
----------------
chandlerc wrote:
> 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...
Eh, copy-pasted comments are almost as bad as copy-pasted code.  But up to you, not a big deal.


================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:107
+/// discriminate the different analysis types used and instantiate this class
+/// with different enumerators as template arguments.
+template <size_t I = static_cast<size_t>(-1)>
----------------
Suggest changing to active voice and then providing a brief example:

> If you want to register multiple loop analysis passes, you'll need to instantiate this type with different values for `I`.  For example:
>
>   MockLoopAnalysisHandleTemplate<0> h0;
>   MockLoopAnalysisHandleTemplate<1> h1;
>   typedef decltype(h0)::Analysis Analysis0;
>   typedef decltype(h1)::Analysis Analysis1;

In fact I could see getting rid of the example, too.

(I got rid of the enum because you only ever use it once, so who cares if you hardcode constants?)


================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:540
+  // The rest don't invalidate,i they only trigger re-runs because we clear the
+  // cache completely.
+
----------------
There's a typo here, but other than that it's fine.  Because phab is awful I am not sure if this is the line the above comments were talking about or what, but I'm sure it's all fine here.


https://reviews.llvm.org/D28292





More information about the llvm-commits mailing list