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

[Chris Diamand via llvm-commits]

chrisdiamand_arm marked 2 inline comments as done.
chrisdiamand_arm added a comment.

Replies inline - cheers!


================
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);
----------------
dberlin wrote:
> Any reason to not just use the block iterator instead of converting operands to blocks repeatedly?
Yep - the index is required for `setIncomingBlock` anyway, so I thought it seemed cleaner to use it throughout instead of mixing indices and iterators.

================
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 &&
----------------
dberlin wrote:
> The number of times you do this makes me wonder if we shouldn't just have a phi_iterator for the basic block.
> 
That would be extremely useful, I think there are 4 here. Probably outside the scope of this patch though...

================
Comment at: lib/Transforms/Scalar/LICM.cpp:421
@@ +420,3 @@
+    }
+  }
+
----------------
dberlin wrote:
> Do we really have no branch redirect utility that does this?
> (I thought we did have one that did this and also updated dominators)
Not that I can find, through lots of recursive grepping. This particular code is quite specific to LICM (or at least loop transformations) anyway - it has to redirect //only// the branches which point outside the subloop. Are there any other passes which replace loop exit blocks that could benefit from this being factored out?


http://reviews.llvm.org/D17203