[PATCH] D71990: [LoopUtils] Better accuracy for getLoopEstimatedTripCount.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:729
+  // weight.
+  uint64_t HeaderBlockWeight = BackedgeTakenWeight + LatchExitWeight;
+  return llvm::divideNearest(HeaderBlockWeight, LatchExitWeight);
----------------
Maybe clearer to emphasize the +1 effect, noting that Trip count is one plus the estimated backedge taken count, and doing

```
uint64_t BackedgeTakenCount = llvm::divideNearest(BackedgeTakenWeight, LatchExitWeight);
return BackedgeTakenCount + 1;
```
(cf. `InnerLoopVectorizer::getOrCreateTripCount(Loop *L)`)


================
Comment at: llvm/test/Transforms/LoopUnroll/peel-loop-conditions-pgo-1.ll:14
 ; CHECK: PEELING loop %for.body with iteration count 2!
-; CHECK: PEELING loop %for.body with iteration count 4!
+; CHECK: PEELING loop %for.body with iteration count 5!
 ; CHECK: llvm.loop.unroll.disable
----------------
It is indeed confusing at first sight, to see that the total number of estimated trip count is 2+5, given branch_weights of 6 and 1 (and thus estimated backedge taken count of 6/1). Perhaps worth clarifying in the above comment about "remained iterations". Should -unroll-peel-max-count=7 be updated?

Note that loop peeling already updates the estimated trip count (from 7 to 5 above), by calling Transforms/Utils/LoopUnrollPeel.cpp's `fixupBranchWeights()`; potential reuse with  D67905's `setLoopEstimatedTripCount()`.


================
Comment at: llvm/unittests/Transforms/Utils/LoopUtilsTest.cpp:1
+//===--------- LoopUtilsTest.cpp - unit tests -----------------------------===//
+//
----------------
Ayal wrote:
> "This file was added." - this file already exists, right?
> What is this trying to check on top of the above fixed lit tests?
> What is this trying to check on top of the above fixed lit tests?

This change seems mostly unrelated to the +1 accuracy fix, right? Suggest to commit it first, testing the original (backedge taken) value, and then modify it along with the +1 patch, similar to above fixes of lit tests.


================
Comment at: llvm/unittests/Transforms/Utils/LoopUtilsTest.cpp:18
+#include "llvm/Transforms/Scalar/LoopPassManager.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/Function.h"
----------------
These should all be in lex order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71990