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

[Anna Thomas via Phabricator via llvm-commits]

anna marked an inline comment as done.
anna added inline comments.


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:294
+static void
+CloneLoopBlocks(Loop *L, Value *NewIter, const bool CreateRemainderLoop,
+                const bool UseEpilogRemainder, BasicBlock *InsertTop,
----------------
reames wrote:
> 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.
Actually, I'm not convinced if the code would be simpler.
So, there are multiple data structures updated when this "new" remainder loop gets generated, apart from the updates to the CFG edges.
After `CloneLoopBlocks`, we'll need to call a modified version of `deleteDeadLoop` (which is currently in LoopDeletion pass). So, apart from breaking the backedge, we'll need to update the LPM to state that the (remainder) loop we just added should be deleted. 


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:546
+      // `Exit` block can have successors).
+      if (BB->getTerminator()->getNumSuccessors())
+        return false;
----------------
reames wrote:
> 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.
We're looking for more than dedicated exits here. This is just a bail out condition to simplify the code for now: the only exit allowed to have successors is the LatchExit.
Also, `getUniqueExitBlocks` works here because we check for the fact that loop is in LoopSimplifyForm (which checks for `hasDedicatedExits`).

However, I think this bail out can be dropped based on your offline suggestion: use LoopSimplify's logic for generating dedicated exits.




================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:567
+  // the latch.
+  const SCEV *BECountSC = SE->getExitCount(L, Latch);
   if (isa<SCEVCouldNotCompute>(BECountSC) ||
----------------
reames wrote:
> 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.
Thanks for bringing this up.
I read the actual SCEV code (comments seem to differ from the code) for `getExitCount` versus `getBackEdgeTakenCount`, and the `getBackEdgeTakenCount` is semantically equivalent to `getExitCount` for all exits in the loop. 

So, I think this will be an NFC for the old code. 




https://reviews.llvm.org/D33001