[PATCH] D33001: [RuntimeUnrolling] Add logic for loops with multiple exit blocks

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:294
+static void
+CloneLoopBlocks(Loop *L, Value *NewIter, const bool CreateRemainderLoop,
+                const bool UseEpilogRemainder, BasicBlock *InsertTop,
----------------
Orthogonal to your change, but this code would be much simpler if we always created the remainder loop and then broke the backedge (removing the loop) if we knew the inserted loop was trivial.


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:429
+  // tree info.
+  for (auto *BB : OtherExits) {
+
----------------
Ok, I'm missing something.  Why do we need this?  Is it just to preserve the dedicated exit property?  If so, wouldn't simply splitting the edge and inserting a trivial edge (without duplication) be a lot simpler?


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:447
+        }
+        NewInst = NewPHI;
+      }
----------------
This assignment is pointless.


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:519
+
   BasicBlock *Exit = L->getUniqueExitBlock(); // successor out of loop
+  // These are exit blocks other than the target of the latch exiting block.
----------------
It looks like you're re-purposing this as the Latch Exit.  Can you rename this and check it in?  It'll make the diff smaller and easier to read.


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:546
+      // `Exit` block can have successors).
+      if (BB->getTerminator()->getNumSuccessors())
+        return false;
----------------
I think this bit of code can be simplified via hasDedicatedExits.

Also, I believe you actually *need* to check that predicate before calling getUniqueExitBlocks.  Reading the comment on that function makes it seem like having dedicated exits is a precondition for that function.


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:567
+  // the latch.
+  const SCEV *BECountSC = SE->getExitCount(L, Latch);
   if (isa<SCEVCouldNotCompute>(BECountSC) ||
----------------
Can you separate and land this change?  It should be NFC for the old code and if it's not, it'd be good to find out now.

And, reading the comments carefully, I'm not sure this is actually NFC.  Aren't the exit count and the backedge taken count defined differently?  (Consider loop whose header executes once and whose backedge is dynamically dead.)

Also, "guaranteed not to exit" doesn't seem to imply guaranteed to exit on the next iteration.


https://reviews.llvm.org/D33001