[PATCH] D61962: [LoopUnroll] Add support for loops with exiting headers and uncond latches.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:48
+STATISTIC(NumUnrolledWithHeader,
+          "Number of loops unrolled (completely or otherwise)");
 
----------------
Same msg as above, is that on purpose?


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:400
+      !CheckHeaderSuccessors(0, 1) && !CheckHeaderSuccessors(1, 0)) {
+    LLVM_DEBUG(dbgs() << "Can't unroll; conditional header must exit the loop");
     return LoopUnrollResult::Unmodified;
----------------
This doesn't check anymore if BI->getParent() is not only a latch block but also an exiting block, or did I miss that? What if it is not?


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:588
+  bool ContinueOnTrue;
+  bool LatchIsExiting = BI->isConditional();
+  BasicBlock *LoopExit = nullptr;
----------------
While somewhat theoretical, what if both targets are the header? I guess there is also an inner-loop case where this would be problematic but that is probably handled elsewhere.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:608
   std::vector<BasicBlock*> Headers;
+  std::vector<BasicBlock *> HeaderSucc;
   std::vector<BasicBlock*> Latches;
----------------
format


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:627
+    };
+    HeaderSucc.push_back(GetHeaderLoopSucc(Header));
+  }
----------------
I don't get why this is a lambda and not simply straight line control flow. I assumed it is recursive but it doesn't seem to be. Isn't the following easier to read?

```
      auto Term = cast<BranchInst>(Header->getTerminator());
      if (Term->isUnconditional() || L->contains(Term->getSuccessor(0))) {
        assert(L->contains(Header->getSuccessor(0)));
        HeaderSucc.push_back(Term->getSuccessor(0))
      } else {
        assert(L->contains(Term->getSuccessor(1)));
        HeaderSucc.push_back(Term->getSuccessor(1))
      }
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61962/new/

https://reviews.llvm.org/D61962