[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:727
 
-  // Divide the count of the backedge by the count of the edge exiting the loop,
-  // rounding to nearest.
-  return llvm::divideNearest(BackedgeTakenWeight, LatchExitWeight);
+  // Trip count is a ratio of the backedge by the count of the edge exiting the
+  // loop, rounding to nearest.
----------------
"Trip count" >> "Estimated backedge taken count"


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:732
+  // For bottom tested loop first iteration has been executed by the time latch
+  // is reached. Thus need to adjust trip count by one.
+  return BackedgeTakenCount + 1;
----------------
Suffice to say that Estimated trip count is one plus estimated backedge taken count.


================
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:
> 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?
OK; just noting that original test estimated the trip count to be 6, and used -unroll-peel-max-count=7, so seemed reasonable to bump this threshold (to 8) given that the estimation bumped (to 7).


================
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:
> ebrevnov wrote:
> > 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?
> OK; just noting that original test estimated the trip count to be 6, and used -unroll-peel-max-count=7, so seemed reasonable to bump this threshold (to 8) given that the estimation bumped (to 7).
OK; just suggesting to extend the existing "remained iterations" comment above, clarifying how many they are.


================
Comment at: llvm/unittests/Transforms/Utils/LoopUtilsTest.cpp:1
+//===--------- LoopUtilsTest.cpp - unit tests -----------------------------===//
+//
----------------
ebrevnov wrote:
> 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.
Sorry, my original request was just to make sure the +1 change is covered by tests, as opposed to NFC patches; the above lit tests seem fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71990