[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/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.
----------------
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.


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

https://reviews.llvm.org/D64553