[PATCH] D52827: [LICM] Make LICM able to hoist phis

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:470
+    BasicBlock *OriginalPreheader = CurLoop->getLoopPreheader();
+    OriginalPreheaderChildren = DT->getNode(OriginalPreheader)->getChildren();
+  }
----------------
Preheader by definition has only one successor which is the loop header. Therefore, `OriginalPreheaderChildren` always has one element which is header's DTNode. So it doesn't look like you even need this field, just take loop header (it doesn't change, right?)

Or am I missing something here?

And btw, if it needs to be a field and a vector, use `SmallVector` instead of `std::vector`.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:559
+
+  BasicBlock *getHoistedBlock(BasicBlock *BB) {
+    // If BB has already been hoisted, return that
----------------
This name is misleading, it doesn't look like something that can change the IR. How about `getOrCreateHoistedBlock`?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:650
+           "Hoisting blocks should not have destroyed preheader");
+    return HoistDestinationMap[BB];
+  }
----------------
Please add verification of `LI` in debug mode just to make sure that we didn't mess up anything (maybe not here, but in some reasonable place).


================
Comment at: lib/Transforms/Scalar/LICM.cpp:672
+  // re-hoisted if they end up not dominating all of their uses.
+  std::list<Instruction *> HoistedInstructions;
+
----------------
Why not `SmallVector` and `push_back`? That wouldn't make any difference in terms of effect but less memory-consuming.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:793
 
+  // If we hoisted instructions to a conditional block they may not dominate
+  // their uses that weren't hoisted. If so make them unconditional by moving
----------------
This is the part I don't understand. If the hoist destination doesn't dominate instruction's users then it also doesn't dominate the original instruction. Is LICM able to hoist to such locations? Is there an example of that?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:804
+                        << ": " << *I << "\n");
+      I->moveBefore(HoistPoint);
+      HoistPoint = I;
----------------
Set `Changed` here.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:805
+      I->moveBefore(HoistPoint);
+      HoistPoint = I;
+    }
----------------
Do we really need to change the hoist point? Why not just hoist of them before preheader's terminator? That would make this code simpler.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1391
   SafetyInfo->insertInstructionTo(Preheader);
-  // Move the new node to the Preheader, before its terminator.
-  I.moveBefore(Preheader->getTerminator());
+  if (isa<PHINode>(I)) {
+    // Move the new node to the end of the phi list in the preheader.
----------------
Braces not needed (here and in some other places like this).


================
Comment at: test/Transforms/LICM/hoist-phi.ll:126
+
+; This is currently too complicated for us to be able to hoist the phi.
+; CHECK-LABEL: @tree_phi
----------------
Please add `TODO:` to the tests you think we should cover in the future, that will make it easier to track if we decide to expand this transform to something more general.


Repository:
  rL LLVM

https://reviews.llvm.org/D52827