[PATCH] D64553: [LoopUtils] Extend the scope of getLoopEstimatedTripCount

[Serguei Katkov via Phabricator via llvm-commits]

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


================
Comment at: llvm/trunk/lib/Transforms/Utils/LoopUtils.cpp:640
+  SmallVector<BasicBlock *, 4> ExitBlocks;
+  L->getUniqueNonLatchExitBlocks(ExitBlocks);
+  if (any_of(ExitBlocks, [](const BasicBlock *EB) {
----------------
skatkov wrote:
> skatkov wrote:
> > efriedma wrote:
> > > I'm seeing `Assertion 'L->hasDedicatedExits() && "getUniqueExitBlocks assumes the loop has canonical form exits!"' failed.` with some out-of-tree code that calls getLoopEstimatedTripCount.  Does this analysis really require that the loop has dedicated exits?  If it does, can we guard this call so getLoopEstimatedTripCount fails gracefully for loops without dedicated exits?
> > > 
> > > I think all the in-tree users of getLoopEstimatedTripCount require dedicated exits for other reasons, so no bug report.
> > Hello Eli,
> > 
> > getUniqueExitBlocks is written in a way that it expects that all predecessors of successor of loop's basic block is in a loop.
> > 
> > I totally fine to add a guard to fail gracefully. Will publish a patch in a couple of hours.
> Hi again,
> 
> It is interesting catch. It seems I introduced a bug while adding getUniqueNonLatchExitBlocks utility function.
> It seems that getUniqueExitBlocksHelper is written in a way it expects that all exits's first predecessor is in a loop and no need to check others predecessors. In case I skip latch I can miss the exit if it has two predecessors: latch and non-latch.
> 
> I will try to write a test and then re-write the getUniqueExitBlocksHelper function. So assert will be eliminated from it.
> 
> Thank you for pointing to this issue.
See D64787


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64553