[PATCH] D35380: [RuntimeUnroll] Add heuristic for unrolling multi-exit loop

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:435
 static bool
-canSafelyUnrollMultiExitLoop(Loop *L, SmallVectorImpl<BasicBlock *> &OtherExits,
+canUnrollMultiExitLoop(Loop *L, SmallVectorImpl<BasicBlock *> &OtherExits,
                              BasicBlock *LatchExit, bool PreserveLCSSA,
----------------
reames wrote:
> Code structure wise, I'd recommend splitting this function into two: one which checks *legality* and one which given a legal case, checks profitability.  Mixing the two needlessly is just confusing.
> 
> This could in fact be done as an NFC change.
I had thought of this approach, but the main problem is that in the caller, there's the chance the user calls profitability without safety checks being done. Granted we can assert for that in the profitability check, but it's expensive.  Perhaps I can place it under an EXPENSIVE_CHECKS option. 


================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:467
+  // All safety constraints have been satisfied. Now check for profitability.
+  auto isMultiExitUnrollingProfitable = [](ArrayRef<BasicBlock *> OtherExits) {
+
----------------
reames wrote:
> Why did you chose this particular heuristic?  What other options did you consider?  (some of this should be in the code itself)
will add details.


https://reviews.llvm.org/D35380