[PATCH] D108108: [LoopPeel] Allow peeling with multiple unreachable-terminated exit blocks.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:103-109
+  L->getUniqueNonLatchExitBlocks(Exits);
+  // The latch must either be the only exiting block or all non-latch exit
+  // blocks are either have an deopt or unreachable terminator.
+  return all_of(Exits, [](const BasicBlock *BB) {
+    return BB->getTerminatingDeoptimizeCall() ||
+           isa<UnreachableInst>(BB->getTerminator());
+  });
----------------
lebedev.ri wrote:
> (As per brief disscussion in IRC) since this is about loop reentry, how about something like this then?
@lebedev.ri I agree that this relaxation would be fine/good for the existing cost heuristics, but as @skatov mentioned, the problem with that case would be updating the branch weights. Unless you have additional concerns, I am planning on going with `unreachable`-terminated exits for now to avoid messing up branch weights. WDYT?


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:105
+  // The latch must either be the only exiting block or all non-latch exit
+  // blocks are either have an deopt or unreachable terminator.
+  return all_of(Exits, [](const BasicBlock *BB) {
----------------
skatkov wrote:
> reames wrote:
> > nikic wrote:
> > > reames wrote:
> > > > Can you add something to this comment to indicate that a) this is a proxy for a strong profile prediction of untaken, and b) this is a profitability check not a legality check?
> > > I would really appreciate an explanation of why we want this heuristic... it's not really obvious to me why this is only beneficial if the other exits are likely not taken.
> > I don't know this is Florian's answer, but mine would be: incrementalism and minimizing blast radius.  We may eventually want to generalize further, but one step at a time.  
> I guess the problem is that LoopPeeling cannot update branch weights for other branches than latch.
> 
> If we can teach to LoopPeeling to update branch weights for non latch branches - we may remove this restriction however it does not look easy task.
> 
> Branch weights to deopt and unreachable is overwritten to be smallest one, so its update is simple (no update).
> 
> This is how I remember a problem.
Thanks for taking a look! My main motivation was as Philip suggested mostly to keep the potential fallout limited and additionally allowing exit blocks terminated by `unreachable` should not negatively impact the existing heuristics.

@skatov thanks for providing the extra context with respect to branch weights, I'll include that in the updated comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108108/new/

https://reviews.llvm.org/D108108