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

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/required fix before submit.



================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:624
 Optional<unsigned> llvm::getLoopEstimatedTripCount(Loop *L) {
-  // Only support loops with a unique exiting block, and a latch.
-  if (!L->getExitingBlock())
-    return None;
+  // Support loops with an exiting latch and other existing exists only
+  // deoptimize.
----------------
skatkov wrote:
> fhahn wrote:
> > Would it make sense to remove the restriction of having a single exit block/only deopt non latch exit by generalizing the function?
> > 
> > Wouldn't' it be reasonable to estimate the trip count by summing up the weights of all exit edges and dividing by the weight of the back-edge to the header? That should naturally cover the deopt cases as well, as they should have very low weights, right?
> > 
> > In any case, we should also update the doc-comment in the header describing the new behavior.
> Hi Florian, thank you for your comment.
> 
> I think that sum of exit weights from different branches is not a good idea. We can compare/use only probabilities not the real values.
> 
> Let's consider the following case, we have a loop with two exits. One of them has weights (10, 1) and the second one (1,1). for simplicity the second number is a weight of exit.
> 
> What will be Trip Count in this case?
> According to your proposed formula it is 5. Looks strange.
> 
> Moreover it is totally fine to scale the weights by some constant. For example, (1,1) is actually the same as (100, 100).
> In this case according to formula trip count will be 0.
> 
> So I think that it is not incorrect and to get the right formula is not really easy task, so I just want to adjust only guard of this formula for the moment.
> 
> With respect to update the header, The header description does not mention the restriction about only one exit now. It is more general and correct. If we can provide estimated trip count we do that. With my additional guard nothing changed.
> 
> If you really want to see that details in the header, please let me know and I will update a header.
I'd ask that we specifically do not extend the scope of this patch.  A separate patch can expand scope, but let's start with something low risk, make sure the caller code is well exercised, then increase the scope.  Working in stages to reduce risk is important.  


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:628
   // Get the branch weights for the loop's backedge.
-  BranchInst *LatchBR =
-      dyn_cast<BranchInst>(L->getLoopLatch()->getTerminator());
-  if (!LatchBR || LatchBR->getNumSuccessors() != 2)
+  BasicBlock *Latch = L->getLoopLatch();
+  BranchInst *LatchBR = dyn_cast<BranchInst>(Latch->getTerminator());
----------------
There's a missing correctness check here.  There's no guarantee that a loop *must* have a unique latch, so you need null check the result before dereferencing.  Non unique latches are non-canonical, so you can just bail on that case.

Note that unique exit implies unique latch previously.


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

https://reviews.llvm.org/D64553