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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

ping



================
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;
----------------
jdoerfert wrote:
> 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?
I think if BI is unconditional in a latch block, the latch cannot be an exiting block as well, as the only successor should be back to the header.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:588
+  bool ContinueOnTrue;
+  bool LatchIsExiting = BI->isConditional();
+  BasicBlock *LoopExit = nullptr;
----------------
jdoerfert wrote:
> 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.
This should be covered by the checkLatchSuccessors check, which would fail in that case I think.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:608
   std::vector<BasicBlock*> Headers;
+  std::vector<BasicBlock *> HeaderSucc;
   std::vector<BasicBlock*> Latches;
----------------
jdoerfert wrote:
> format
`std::vector<BasicBlock *>` is what clang-format produces. I'll adjust the line above, because it looks extremely inconsistent.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:627
+    };
+    HeaderSucc.push_back(GetHeaderLoopSucc(Header));
+  }
----------------
jdoerfert wrote:
> 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))
>       }
> ```
Yep thanks. I think an earlier version of the patch used it elsewhere too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61962