[PATCH] D17203: [LICM] Sink entire inner loops.

[Daniel Berlin via llvm-commits]

dberlin added a subscriber: dberlin.
dberlin added a comment.

Generally looks good to me.
I can't approve it, though.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:359
@@ +358,3 @@
+  for (auto II = BB->begin(); (PN = dyn_cast<PHINode>(II)); ++II) {
+    for (unsigned Idx = 0; Idx < PN->getNumOperands(); ++Idx) {
+      BasicBlock *Incoming = PN->getIncomingBlock(Idx);
----------------
Any reason to not just use the block iterator instead of converting operands to blocks repeatedly?

================
Comment at: lib/Transforms/Scalar/LICM.cpp:374
@@ +373,3 @@
+  Loop::iterator it = std::find(SubLoops.begin(), SubLoops.end(), SubLoop);
+  assert(it != SubLoops.end() && *it == SubLoop);
+  SubLoop = Parent->removeChildLoop(it);
----------------
Please add a message

================
Comment at: lib/Transforms/Scalar/LICM.cpp:394
@@ +393,3 @@
+  PHINode *PN;
+  for (auto II = OldExitBlock->begin(); (PN = dyn_cast<PHINode>(II)); ++II) {
+    if (PN->getNumOperands() == 1 &&
----------------
The number of times you do this makes me wonder if we shouldn't just have a phi_iterator for the basic block.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:421
@@ +420,3 @@
+    }
+  }
+
----------------
Do we really have no branch redirect utility that does this?
(I thought we did have one that did this and also updated dominators)


http://reviews.llvm.org/D17203