[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