[PATCH] D17203: [LICM] Hoist and sink entire inner loops.
James Molloy via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 15 09:15:02 PST 2016
jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.
Hi Chris,
Thanks for working on this! This is a very impressively written patch. Most of my comments are to do with coding style and using more of LLVM's frameworks to avoid using helper functions.
Cheers,
James
================
Comment at: include/llvm/Analysis/LoopInfo.h:369
@@ -368,1 +368,3 @@
+ bool isLoopInvariantOutsideSubLoop(const Value *V,
+ const Loop *Sub) const;
----------------
This is a very public header, so please add good doxygen comments.
================
Comment at: lib/Transforms/Scalar/LICM.cpp:331
@@ +330,3 @@
+ for (auto II = ExitBlock->begin(); (PN = dyn_cast<PHINode>(II)); ++II) {
+ if (PN && PN->getNumOperands() == 1 &&
+ SubLoop->contains(PN->getIncomingBlock(0))) {
----------------
don't need to check PN here because the loop condition has already checked it.
================
Comment at: lib/Transforms/Scalar/LICM.cpp:344
@@ +343,3 @@
+ for (BasicBlock *BB : SubLoop->blocks()) {
+ for (Instruction &I : BB->getInstList()) {
+ if (!dyn_cast<PHINode>(&I) &&
----------------
s/BB->getInstList()/*BB/
================
Comment at: lib/Transforms/Scalar/LICM.cpp:345
@@ +344,3 @@
+ for (Instruction &I : BB->getInstList()) {
+ if (!dyn_cast<PHINode>(&I) &&
+ !dyn_cast<BranchInst>(&I) &&
----------------
instead of !dyn_cast, use !isa<PHINode>().
================
Comment at: lib/Transforms/Scalar/LICM.cpp:383
@@ +382,3 @@
+ // means the loop can't be hoisted.
+ // TODO: Only do this loop once, share result between sinking and hoisting.
+ for (BasicBlock *BB : SubLoop->blocks()) {
----------------
s/TODO/FIXME/
================
Comment at: lib/Transforms/Scalar/LICM.cpp:392
@@ +391,3 @@
+ return false;
+ } else if (dyn_cast<PHINode>(&I) ||
+ dyn_cast<BranchInst>(&I)) {
----------------
Same comment as above: use isa<> instead of dyn_cast<> here if you're not using the result.
================
Comment at: lib/Transforms/Scalar/LICM.cpp:409
@@ +408,3 @@
+template <typename PredTy>
+static void replaceSuccessor(BasicBlock *BB, PredTy Predicate,
+ BasicBlock *NewSucc) {
----------------
I'd rename this "replaceSuccessorIf", which implies the predicate. Functions that take predicates are also nicer to call if the predicate is last (it makes formatting a lambda easier).
Actually, I'd call it "replaceFirstSuccessorIf", as it doesn't replace multiple successors.
================
Comment at: lib/Transforms/Scalar/LICM.cpp:412
@@ +411,3 @@
+ auto Term = BB->getTerminator();
+ for (unsigned i = 0; i < Term->getNumSuccessors(); ++i) {
+ if (Predicate(Term->getSuccessor(i))) {
----------------
Capital "I" is the convention.
================
Comment at: lib/Transforms/Scalar/LICM.cpp:461
@@ +460,3 @@
+ auto Inst = dyn_cast<Instruction>(PN->getIncomingValue(0));
+ if (Inst && SubLoop->contains(Inst)) {
+ PhisToMove.push_back(PN);
----------------
Single-line if statements should have their braces elided.
================
Comment at: lib/Transforms/Scalar/LICM.cpp:468
@@ +467,3 @@
+ PN->removeFromParent();
+ NewExitBlock->getInstList().push_back(PN);
+ }
----------------
You should never need to directly modify InstList. Instead, just do:
PN->moveBefore(NewExitBlock->getTerminator())
(and remove PN->removeFromParent()).
OOI, it looks like NewExitBlock doesn't have a Terminator instruction at this point. I find it's always easier to make the block well-formed (add a terminator) as soon as possible, as it makes dumps, sanity checks and insertions (like this) easier.
================
Comment at: lib/Transforms/Scalar/LICM.cpp:473
@@ +472,3 @@
+ BasicBlock *SubLoopPredecessor = SubLoop->getLoopPredecessor();
+ replaceSuccessor(SubLoopPredecessor, [SubLoopHeader](BasicBlock *BB) {
+ return BB == SubLoopHeader;
----------------
You don't need this helper function:
SubLoopPredecessor->getTerminator()->replaceUsesOfWith(SubLoopHeader, OldExitBlock);
================
Comment at: lib/Transforms/Scalar/LICM.cpp:478
@@ +477,3 @@
+ auto ExitingBlock = SubLoop->getExitingBlock();
+ replacePhiUses(OldExitBlock, [ExitingBlock](BasicBlock *BB) {
+ return BB == ExitingBlock;
----------------
for (auto *PN = OldExitBlock.begin(); isa<PHINode>(PN); ++PN)
PN->replaceUsesOfWith(ExitingBlock, SubLoopPredecessor);
================
Comment at: lib/Transforms/Scalar/LICM.cpp:483
@@ +482,3 @@
+ // Branch from the subloop's exiting block to the new exit block.
+ replaceSuccessor(ExitingBlock, [SubLoop](BasicBlock *BB) {
+ return !SubLoop->contains(BB);
----------------
You can use RAUW just like in line 473 to remove the need for the helper.
================
Comment at: lib/Transforms/Scalar/LICM.cpp:487
@@ +486,3 @@
+
+ for (auto BBI = SubLoop->block_begin(); BBI != SubLoop->block_end(); ++BBI) {
+ CurLoop->removeBlockFromLoop(*BBI);
----------------
No braces around single-line statements.
Also:
for (auto *BB : SubLoop->blocks())
CurLoop->removeBlockFromLoop(BB);
================
Comment at: lib/Transforms/Scalar/LICM.cpp:512
@@ +511,3 @@
+ PreheaderTerminator->removeFromParent();
+ NewExitBlock->getInstList().push_back(PreheaderTerminator);
+
----------------
Assuming you've added a dummy terminator before now to keep NewExitBlock well-formed, you can simply do:
PreheaderTerminator->moveBefore(NewExitBlock->getTerminator());
NewExitBlock->getTerminator()->eraseFromParent();
================
Comment at: lib/Transforms/Scalar/LICM.cpp:533
@@ +532,3 @@
+// Insert the subloop after the outer loop's exit block. Then, sink all the
+// original exit block's instructions down to the subloop's exit block,
+// *except* phi nodes.
----------------
Algorithmically this might be easier done by first splitting the outer loop's exit block after all PHI nodes then inserting the subloop in between. (see BasicBlock::split)
================
Comment at: lib/Transforms/Scalar/LICM.cpp:596
@@ +595,3 @@
+ for (BasicBlock *BB : SubLoop->blocks()) {
+ for (Instruction &I : BB->getInstList()) {
+ for (unsigned Idx = 0; Idx < I.getNumOperands(); ++Idx) {
----------------
*BB, not BB->getInstList()
================
Comment at: lib/Transforms/Scalar/LICM.cpp:659
@@ +658,3 @@
+ SafetyInfo);
+ SubLoop->verifyLoop();
+ CurLoop->verifyLoop();
----------------
These should be inside DEBUG() macros for release builds.
================
Comment at: lib/Transforms/Scalar/LICM.cpp:1437
@@ -1070,4 +1436,3 @@
void LICM::cloneBasicBlockAnalysis(BasicBlock *From, BasicBlock *To, Loop *L) {
- AliasSetTracker *AST = LoopToAliasSetMap.lookup(L);
- if (!AST)
- return;
+ auto ASTI = LoopToAliasSetMap.find(L);
+ if (ASTI != LoopToAliasSetMap.end()) {
----------------
I'm not sure I understand why these have been changed?
================
Comment at: test/Analysis/ScalarEvolution/2012-03-26-LoadConstant.ll:1
@@ -1,2 +1,2 @@
-; RUN: opt < %s -basicaa -globalopt -instcombine -loop-rotate -licm -instcombine -indvars -loop-deletion -constmerge -S | FileCheck %s
+; RUN: opt < %s -basicaa -globalopt -instcombine -loop-rotate -licm -instcombine -indvars -constmerge -loop-deletion -S | FileCheck %s
; PR11882: ComputeLoadConstantCompareExitLimit crash.
----------------
Why have these orders changed?
================
Comment at: test/Transforms/LICM/inner-loop-dont-sink.ll:5
@@ +4,3 @@
+entry:
+; CHECK-LABEL: entry:
+; CHECK-NEXT: br label %outer.header
----------------
We generally only use CHECK-LABEL: for delineating between multiple testcases. Its only purpose is to stop llvm-lit running from one testcase to another.
As you've only got one testcase here, you should just use CHECK:.
http://reviews.llvm.org/D17203
More information about the llvm-commits
mailing list