[PATCH] D61962: [LoopUnroll] Add support for loops with exiting headers and uncond latches.
Johannes Doerfert via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 12 12:46:04 PDT 2019
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
More information about the llvm-commits
mailing list