[PATCH] D28292: [PM] Rewrite the loop pass manager to use a worklist and augmented run arguments much like the CGSCC pass manager.

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

This is a large patch, but here's a first round of comments (inline).
I personally like a lot the idea of having the analyses always available (with my pass writer hat on).

Also:

1. You propose as `FIXME` not using RPO. Do you have data/evidence that a different visitation order actually matters? I'm not entirely convinced about it (but I haven't thought about the problem very carefully so).
2. You removed a call to `getContext().yield()`. Do you happen to know why it was introduced in the first place?



================
Comment at: include/llvm/Analysis/LoopPassManager.h:56-58
+// Forward declarations of a update tracking and analysis result tracking
+// structures used in the API of loop passes that work within this
+// infrastructuer.
----------------
Nit: infrastructure.


================
Comment at: include/llvm/Analysis/LoopPassManager.h:86-90
+/// \brief The CGSCC pass manager.
+///
+/// See the documentation for the PassManager template for details. It runs
+/// a sequency of SCC passes over each SCC that the manager is run over. This
+/// typedef serves as a convenient way to refer to this construct.
----------------
ehm, is this comment correct?
Shouldn't it say `The Loop pass manager` ?


================
Comment at: include/llvm/Analysis/LoopPassManager.h:88-90
+/// See the documentation for the PassManager template for details. It runs
+/// a sequency of SCC passes over each SCC that the manager is run over. This
+/// typedef serves as a convenient way to refer to this construct.
----------------
Also doesn't it run over a sequency of `Loop` passes?


================
Comment at: include/llvm/Analysis/LoopPassManager.h:154-155
+
+  /// \brief Handler for invalidation of the Module.
+  ///
+  /// If the proxy analysis itself is preserved, then we assume that the set of
----------------
invalidation of the Module or the Function?


================
Comment at: include/llvm/Analysis/LoopPassManager.h:193-194
+namespace internal {
+/// Helper to implement correct appending of loops onto a worklist.
+///
+/// The worklist is a LIFO data structure and we want the observed order to be
----------------
I think "correct" is unneeded. I assume every algorithm implemented here is supposed to be correct.


================
Comment at: include/llvm/Analysis/LoopPassManager.h:197-199
+template <typename RangeT>
+inline void appendLoopsInRPO(RangeT &&Loops,
+                             SmallPriorityWorklist<Loop *, 4> &Worklist) {
----------------
It could be just me, but I never saw RPO on a tree (only on graphs). Therefore, maybe this function can be called `appendLoopsInPreorder` as it's what the function is doing (and move the comment explaining why the two forms are equivalent on the top of the function).


================
Comment at: include/llvm/Analysis/LoopPassManager.h:200
+                             SmallPriorityWorklist<Loop *, 4> &Worklist) {
+  // We want a reverse postorder traveral of the loop nest to feed into the
+  // worklist because we pop off of its back and want to observe the loops
----------------
traveral -> traversal.


================
Comment at: include/llvm/Analysis/LoopPassManager.h:313-321
+  template <typename LoopPassT> friend class llvm::FunctionToLoopPassAdaptor;
+
+  SmallPriorityWorklist<Loop *, 4> &Worklist;
+
+  LoopAnalysisManager &LAM;
+
+  Loop *CurrentL;
----------------
Style nit: all these newlines between member variables are inconsistent with what you use elsewhere (I don't care which style you use, but pick one and stick with it).


================
Comment at: lib/Analysis/LoopPassManager.cpp:71
+
+  // Invaliadtion was handled after each pass in the above loop for the current
+  // SCC. Therefore, the remaining analysis results in the AnalysisManager are
----------------
Invaliadtion -> Invalidation.


https://reviews.llvm.org/D28292