[PATCH] D36157: [PM] Split LoopUnrollPass and make partial unroller a function pass

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: include/llvm/Transforms/Scalar/LoopUnrollPass.h:24
 public:
-  /// Create an instance of the loop unroll pass that will support both full
-  /// and partial unrolling.
-  ///
-  /// This uses the target information (or flags) to control the thresholds for
-  /// different unrolling stategies but supports all of them.
-  static LoopUnrollPass create(int OptLevel = 2) {
-    return LoopUnrollPass(/*AllowPartialUnrolling*/ true, OptLevel);
-  }
-
-  /// Create an instance of the loop unroll pass that only does full loop
-  /// unrolling.
-  ///
   /// This will disable any runtime or partial unrolling.
+  explicit LoopFullUnrollPass(int OptLevel = 2) : OptLevel(OptLevel) {}
----------------
This comment seems a bit stale now.


================
Comment at: include/llvm/Transforms/Scalar/LoopUnrollPass.h:31
+
+/// Loop unroll pass that will support both full and partial unrolling.
+class LoopUnrollPass : public PassInfoMixin<LoopUnrollPass> {
----------------
I think it would be good to talk about the fact that this is a function pass rather than a loop pass in order to have access to analyses and control over the loop nest.

Also (see below for details) probably should mention that it will additionally put loops into canonical (simplified + LCSSA) form


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:1217-1218
+template <typename RangeT>
+static void appendLoopsToWorklist(RangeT &&Loops,
+                                  SmallVector<Loop *, 8> &Worklist) {
+  // We use an internal worklist to build up the preorder traversal without
----------------
Since you always call this with an empty vector, maybe just return it by value?


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:1247
+  auto &ORE = AM.getResult<OptimizationRemarkEmitterAnalysis>(F);
+
+  SmallVector<Loop *, 8> Worklist;
----------------
I would directly simplify the loops and form LCSSA here so that you have the requirements for unrolling. That's similar to what the vectorizer does, although it doesn't seem to care about LCSSA and the unroller will.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:1249-1253
+  appendLoopsToWorklist(LI, Worklist);
+
+  bool Changed = false;
+  while (!Worklist.empty()) {
+    Loop &L = *Worklist.pop_back_val();
----------------
Mention somewhere the ordering?


https://reviews.llvm.org/D36157