[PATCH] D17203: [LICM] Hoist and sink entire inner loops.

[Chris Diamand via llvm-commits]

chrisdiamand_arm added a comment.

Hi, thanks for taking a look at this! Comments inline.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:354
@@ +353,3 @@
+
+  // Don't hoist if it has multiple exit or entry blocks.
+  if (!SubLoop->getExitBlock() || !SubLoop->getLoopPreheader()) {
----------------
reames wrote:
> Given these two sets of checks are repeated, a helper function would be good.  Alternatively, is isLoopSimplifyForm sufficient?
`isLoopSimplifyForm` allows multiple exit blocks, so it's not sufficient as currently written. LICM can sink individual instructions to multiple exit blocks IIRC, but it has to duplicate the instruction for each exit block. I'm not sure we'd want to duplicate entire inner loops though?

================
Comment at: lib/Transforms/Scalar/LICM.cpp:361
@@ +360,3 @@
+
+  if (!DT->dominates(SubLoop->getHeader(), CurLoop->getExitBlock())) {
+    DEBUG(dbgs() << "Not hoisting subloop: " << SubLoop->getHeader()->getName()
----------------
reames wrote:
> Do we have any guarantee at this point that CurLoop has only one exit?  If so, assert it.  
Good point, this should be checked by `canHoistSubLoop`. I'll add an assert, too.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:379
@@ +378,3 @@
+        return false;
+      } else if (isa<PHINode>(&I) || isa<BranchInst>(&I)) {
+        continue;
----------------
reames wrote:
> Everywhere you have isa<BranchInst> you probably want to introduce handle all terminators except invokes.  
I'm not sure about this - wouldn't that allow something like an indirect branch to a function with side-effects to be hoisted?

================
Comment at: lib/Transforms/Scalar/LICM.cpp:384
@@ +383,3 @@
+        return false;
+      } else if (!isSafeToExecuteUnconditionally(I, DT, TLI, CurLoop, SafetyInfo,
+                                 CurLoop->getLoopPreheader()->getTerminator())) {
----------------
reames wrote:
> This is a *really* expensive way to phrase this query.  It'll be general, but slow.  Can you look for an alternate way to express this for the entire loop in one go?
I think all of this stuff has to be checked for every instruction at some point. I think some checks are redundant though - `canSinkOrHoistInst` actually calls `isSafeToExecuteUnconditionally`, so the second call is redundant, for example (I just have both because that's what the original hoisting code does).

Some of this stuff is calculated during normal single instruction hoisting/sinking, so I'll investigate if information from that can be reused somehow.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:609
@@ +608,3 @@
+  if (inSubLoop(BB, CurLoop, LI)) {
+    BasicBlock *BB = N->getBlock();
+    Loop *SubLoop = LI->getLoopFor(BB);
----------------
reames wrote:
> BB is already available in this scope.
Good point :)

================
Comment at: lib/Transforms/Scalar/LICM.cpp:622
@@ +621,3 @@
+        Changed = true;
+        // runOnLoop uses the subloop list to find out what to free. Because
+        // SubLoop is no longer in that list, free it here instead.
----------------
reames wrote:
> I don't see that your updating DT here.  This is problematic since we'll be walking a stale tree.
> 
> 
> 
Ok. I'll take a look at the other passes you mentioned to see what they do about this.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:720
@@ +719,3 @@
+        Changed = true;
+        // runOnLoop uses the subloop list to find out what to free. Because
+        // SubLoop is no longer in that list, free it here instead.
----------------
reames wrote:
> FYI, this part in particular looks really really suspect.  I'm not quite sure what the right way to solve this is, but this probably isn't it.  :)
This bit was pretty tricky, and I agree it's not ideal. Here I used `LP` to access `LICM::deleteAnalysisSubloop`, which frees the AST (maybe it looks like I'm trying to tell the LoopPassManager about the deleted loop here?). An alternative would be to make `sinkRegion` and `hoistRegion` methods of `LICM`.

Either way, the AST management has to change a bit, otherwise hoisted subloops' ASTs don't get freed.


http://reviews.llvm.org/D17203