[PATCH] D16257: PM: Implement a basic loop pass manager

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

OMG, the delaays reviewing. So sorry.

Most of this is excellent. I think the tricky question is around handling the mutation of the loop nest correctly. I wonder if it'd be worthwhile to just rip handling of that out entirely in order to land the rest and then support mutation as a follow-up patch so we can look at the alternatives there without having so much boiler plate hanging in a patch?


================
Comment at: include/llvm/Analysis/LoopInfo.h:473-478
@@ +472,8 @@
+  StringRef getName() const {
+    // TODO: Is the name of the header block sufficient, or should we have
+    // something like "<function> loop <header block>"?
+    if (BasicBlock *Header = getHeader())
+      if (Header->hasName())
+        return Header->getName();
+    return "<unnamed loop>";
+  }
----------------
I think this is fine, the caller can dig out more info as needed.

================
Comment at: include/llvm/Analysis/LoopPassManager.h:41-44
@@ +40,6 @@
+
+template <> class IRUnitTraits<Loop> {
+public:
+  static bool hasBeenInvalidated(Loop &IR) { return IR.isInvalid(); }
+};
+
----------------
This trait isn't really commented, but I see what you're using it for (at least I think so -- to stop the LoopPassManager when the current loop gets deleted?).

That technique feels a bit clumsy to me. What do you think about instead expanding the LoopPassManager to just be its own beast and have loop-specific logic where it halts the passes if the loop itself goes away, and logs helpfully? Maybe we could sink some of the boilerplate into a base class so it is reasonably easy to define a custom pass manager here?

================
Comment at: include/llvm/Analysis/LoopPassManager.h:214-218
@@ +213,7 @@
+
+  void enqueueLoop(Loop *L, std::deque<Loop *> &LQ) {
+    LQ.push_back(L);
+    for (auto *NestedLoop : reverse(*L))
+      enqueueLoop(NestedLoop, LQ);
+  }
+
----------------
Why not a worklist rather than explicit recursion? Either way, either inline the code or if still recursive use a lambda rather than a helper function?

================
Comment at: include/llvm/Analysis/LoopPassManager.h:232
@@ +231,3 @@
+
+    std::deque<Loop *> LQ;
+    for (auto *L : reverse(LI))
----------------
Why a deque? It looks like we just push_back, so a vector (or smallvector) would seem more appropriate?

================
Comment at: include/llvm/Analysis/LoopPassManager.h:233
@@ +232,3 @@
+    std::deque<Loop *> LQ;
+    for (auto *L : reverse(LI))
+      enqueueLoop(L, LQ);
----------------
Some comment about the reverse?

================
Comment at: include/llvm/Analysis/LoopPassManager.h:236-237
@@ +235,4 @@
+
+    for (auto *L : reverse(LQ)) {
+      PreservedAnalyses PassPA = Pass.run(*L, LAM);
+
----------------
It's a bit odd that you don't directly handle updates to the loop structure here. I feel like you could by potentially avoiding the queue above entirely and directly sinking this into the recursive walk over the LoopInfo so that as passes update LoopInfo, in turn your traversal is kept up-to-date.

As part of that, maybe check that passes preserve LoopInfo here?


Repository:
  rL LLVM

http://reviews.llvm.org/D16257