[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 11:42:08 PST 2017
jlebar added a comment.
I've gotten through everything except the test bodies. Overall this seems pretty straightforward -- or at least, it's a pretty straightforward application of what you've developed previously.
================
Comment at: include/llvm/Analysis/LoopInfo.h:856
-/// \brief Pass for printing a loop's contents as LLVM's text IR assembly.
-class PrintLoopPass : public PassInfoMixin<PrintLoopPass> {
- raw_ostream &OS;
- std::string Banner;
-
-public:
- PrintLoopPass();
- PrintLoopPass(raw_ostream &OS, const std::string &Banner = "");
-
- PreservedAnalyses run(Loop &L, AnalysisManager<Loop> &);
-};
+/// Method to print a loop's contents as LLVM's text IR assembly.
+void printLoop(Loop &L, raw_ostream &OS, const std::string &Banner = "");
----------------
Nit, "function"?
================
Comment at: include/llvm/Analysis/LoopPassManager.h:16
+/// provide a set of core guarantees:
+/// 1) Loops are, where possible in simplified form.
+/// 2) Loops are *always* in LCSSA form.
----------------
comma after "possible"
================
Comment at: include/llvm/Analysis/LoopPassManager.h:25
+/// 5) Loop passes run over each loop in the loop nest from the inner most to
+/// the outer most. Specifically, all inner loops are processed before
+/// passes run over outer loops. When running the pipeline across an inner
----------------
My usage dictionary says "innermost" and "outermost".
================
Comment at: include/llvm/Analysis/LoopPassManager.h:58
+// structures used in the API of loop passes that work within this
+// infrastructuer.
+class LPMUpdateResult;
----------------
sp
================
Comment at: include/llvm/Analysis/LoopPassManager.h:93
+ LPMUpdateResult &>
+ LoopPassManager;
+
----------------
Ah, I wasn't sold on `typedef PassManager<Function> FunctionPassManager`, but now I am.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:95
+
+/// A partial specialization of the require analysis template pass.
+template <typename AnalysisT>
----------------
Can we add a bit of verbiage about what we're trying to accomplish here? It's not obvious to me.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:126
+public:
+ explicit Result(LoopAnalysisManager &InnerAM, LoopInfo &LI)
+ : InnerAM(&InnerAM), LI(&LI) {}
----------------
Nit, Do you really want explicit on this two-arg constructor?
================
Comment at: include/llvm/Analysis/LoopPassManager.h:169
+private:
+ LoopAnalysisManager *InnerAM;
+ LoopInfo *LI;
----------------
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.
================
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.
+///
----------------
, so
================
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:
> , 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.
================
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;
----------------
Not sure this comment is necessary.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:209
+ // We walk the initial sequence of loops in reverse because we generally want
+ // to visit defs before uses between unrelated loop nests and the worklist is
+ // LIFO.
----------------
s/between unrelated loop nests//
================
Comment at: include/llvm/Analysis/LoopPassManager.h:222
+
+ // Now move the postorder sequence into a priority worklist.
+ Worklist.insert(std::move(PreOrderLoops));
----------------
s/a/the/? Or remove the comment, I think it's pretty clear.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:245
+
+/// This class provides an interface for update the loop pass manager based on
+/// mutations to the loop nest.
----------------
for updating
================
Comment at: include/llvm/Analysis/LoopPassManager.h:248
+///
+/// Loop passes which modify the loop nest should ensure they use one of these
+/// APIs to update LPM infrastructure.
----------------
s/ensure they//
================
Comment at: include/llvm/Analysis/LoopPassManager.h:251
+///
+/// Note that this class cannot be directly constructed, and is instead
+/// provided as an argument to each loop pass.
----------------
I mean, someone has to construct it. Maybe it's sufficient to combine with the above para:
> An instance of this class is passed as an argument to each loop pass, and loop passes should use it to update the LPM infrastructure if they modify the loop nest.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:253
+/// provided as an argument to each loop pass.
+class LPMUpdateResult {
+public:
----------------
This is kind of a weird name -- at least as described above, it doesn't sound like a "result". LPMUpdater?
================
Comment at: include/llvm/Analysis/LoopPassManager.h:261
+ /// directly
+ /// as it may no longer be in a valid state.
+ bool skipCurrentLoop() const { return SkipCurrentLoop; }
----------------
reflow comment
================
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:
> reflow comment
, as
================
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:
> > 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.
?
================
Comment at: include/llvm/Analysis/LoopPassManager.h:285
+ ///
+ /// The \p NewChildLoops must contain only the immediate children. Any nested
+ /// loops within them will be visited in postorder as usual for the loop pass
----------------
s/The//
================
Comment at: include/llvm/Analysis/LoopPassManager.h:289
+ void addChildLoops(ArrayRef<Loop *> NewChildLoops) {
+ // Insert ourselves back into the worklist first as this loop should be
+ // revisited after all the children have been processed.
----------------
, as
================
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.
----------------
s/,/ --/ or s/, i/: I/
================
Comment at: include/llvm/Analysis/LoopPassManager.h:303
+ ///
+ /// The \p NewSibLoops must only contain the immediate sibling loops. Any
+ /// nested loops within them will be visited in postorder as usual for the
----------------
s/The//
================
Comment at: include/llvm/Analysis/LoopPassManager.h:306
+ /// loop pass manager.
+ void addSiblingLoops(ArrayRef<Loop *> NewSibLoops) {
+ internal::appendLoopsToWorklist(NewSibLoops, Worklist);
----------------
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?
================
Comment at: include/llvm/Analysis/LoopPassManager.h:307
+ void addSiblingLoops(ArrayRef<Loop *> NewSibLoops) {
+ internal::appendLoopsToWorklist(NewSibLoops, Worklist);
+
----------------
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?
================
Comment at: include/llvm/Analysis/LoopPassManager.h:309
+
+ // No need to skip the current loop or revisit it as sibling loops
+ // shouldn't impact anything.
----------------
, as
================
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;
----------------
This changes if you call addSiblingLoops or addChildLoops, so it doesn't seem so fixed. Maybe we can just say what it is 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);
----------------
Update this comment if we change the name of LPMUpdateResult.
================
Comment at: include/llvm/Analysis/LoopPassManager.h:438
+
+/// \brief Pass for printing a loop's contents as LLVM's text IR assembly.
+class PrintLoopPass : public PassInfoMixin<PrintLoopPass> {
----------------
Perhaps simply
> Pass for printing a loop's contents as textual IR.
================
Comment at: lib/Analysis/LoopPassManager.cpp:31
+/// Explicitly specialize the pass manager run method to handle call graph
+/// updates.
----------------
s/manager/manager's/
================
Comment at: lib/Analysis/LoopPassManager.cpp:32
+/// Explicitly specialize the pass manager run method to handle call graph
+/// updates.
template <>
----------------
It's not call graph updates we care about, right? It's the addition / removal of loops?
================
Comment at: lib/Analysis/LoopPassManager.cpp:41
+
+ if (DebugLogging)
+ dbgs() << "Starting Loop pass manager run.\n";
----------------
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?
================
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
----------------
What do SCCs have to do with this? (Stale comment?)
================
Comment at: lib/Analysis/LoopPassManager.cpp:92
+ // up reversing the preorder sequence. However, it happens that the loop nest
+ // roots are in reverse order within the LoopInfo object so we just walk
+ // forward here.
----------------
s/ so/. So/
================
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.
----------------
Can you rephrase the "Because" sentence?
================
Comment at: lib/Analysis/LoopPassManager.cpp:136
+ // manager. This also should catch any attempt to use the analysis manager
+ // via this proxy prior to it being rebuilt.
+ // FIXME: This last property isn't very nice. Most analyses try to remain
----------------
its being
================
Comment at: lib/Analysis/LoopPassManager.cpp:141
+
+ // New return true to indicate this *is* invalid and a fresh proxy result
+ // needs to be built.
----------------
s/New/Now/? Otherwise I don't know what you mean.
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:40
+ typename... ExtraArgTs>
+class MockAnalysisHandleTemplateBase {
public:
----------------
Maybe just MockAnalysisHandleBase?
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:78
+protected:
+ /// The base provides a method the derived class constructor can use to set
+ /// up default actions. We can't do this directly because our constructor is
----------------
s/The base provides/We provide/
Or perhaps just
> Derived classes should call this in their constructor to set up default mock actions. (We can't do this in our constructor because this has to run after the DerivedT is constructed.)
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:100
-class TestLoopPass {
- std::vector<StringRef> &VisitedLoops;
- int &AnalyzedBlockCount;
- bool OnlyUseCachedResults;
+template <size_t I = static_cast<size_t>(-1)>
+struct MockLoopAnalysisHandleTemplate
----------------
Comment what is `I`
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:152
+ /// up default actions. We can't do this directly because our constructor is
+ /// run before the derived object is constructed.
+ void setDefaults() {
----------------
ibid
================
Comment at: unittests/Analysis/LoopPassManagerTest.cpp:184
+ return Name == arg.getName();
+}
+
----------------
I think some judicious comments are called for above, especially given that your readers may be totally unfamiliar with gmock.
https://reviews.llvm.org/D28292
More information about the llvm-commits
mailing list