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

[Evgeniy via Phabricator via llvm-commits]

ebrevnov marked 5 inline comments as done.
ebrevnov added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:729
+  // weight.
+  uint64_t HeaderBlockWeight = BackedgeTakenWeight + LatchExitWeight;
+  return llvm::divideNearest(HeaderBlockWeight, LatchExitWeight);
----------------
Ayal wrote:
> 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)`)
Sure, works for me. In fact, this +1 approach was used in original patch :-)


================
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
----------------
Ayal wrote:
> 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()`.
> Should -unroll-peel-max-count=7 be updated?
Not sure I understand why it should be updated. Loop has 7 estimated iterations and we aim at peeling 2 iterations. Thus -unroll-peel-max-count=7 seems reasonable to me.



================
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
----------------
ebrevnov wrote:
> Ayal wrote:
> > 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()`.
> > Should -unroll-peel-max-count=7 be updated?
> Not sure I understand why it should be updated. Loop has 7 estimated iterations and we aim at peeling 2 iterations. Thus -unroll-peel-max-count=7 seems reasonable to me.
> 
> 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).

Even if I clarify here it won't help a lot since there are bunch of other similar places. Probably it's better to add explanation to getLoopEstimatedTripCount?


================
Comment at: llvm/unittests/Transforms/Utils/LoopUtilsTest.cpp:1
+//===--------- LoopUtilsTest.cpp - unit tests -----------------------------===//
+//
----------------
Ayal wrote:
> 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.
> What is this trying to check on top of the above fixed lit tests?
I believe you requested to add such test at some time. Anyway it's better to have such test than not :-). Will extract this change to a separate review.


================
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"
----------------
Ayal wrote:
> These should all be in lex order.
Will fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71990