[PATCH] D67905: [LV] Vectorizer should adjust trip count in profile information

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1040
+  uint64_t OrigBackedgeTakenWeight = 0;
+  uint64_t OrigLoopEntryWeight = 0;
+  auto *OrigLoopLatchBranch = OrigLoop->getLoopLatch()->getTerminator();
----------------
Ayal wrote:
> ebrevnov wrote:
> > Ayal wrote:
> > > Worth commenting that OrigLoopEntryWeight also holds OrigLoopExitWeight, which is more clearly the weight associated with the (exit direction of the) latch branch.
> > IMHO instead of trying to clarify with a comment we better find self descriptive name for such a simple and commonly used thing.  Strictly speaking OrigLoopEntryWeight != OrigLoopExitWeight. Do you find OrigBackEdgeExitWeight good enough?
> > IMHO instead of trying to clarify with a comment we better find self descriptive name for such a simple and commonly used thing.
> Definitely agree with (the preference of) finding self descriptive variable names.
> 
> > Strictly speaking OrigLoopEntryWeight != OrigLoopExitWeight"
> How so, given that OrigLoop has a single-entry, and an "expected" single-exit: there may be other "side/deopt" exits, but these are expected to have zero weight. E.g., when computing LoopHeaderWeight above, adding LoopEntryWeight (instead of LoopExitWeight) to BackEdgeTakenWeight seems more logical. 
> 
> > Do you find OrigBackEdgeExitWeight good enough?
> Strictly speaking, a BackEdge cannot exit: it is an edge going from a latch block to a header block, and "BackEdgeTaken" is the number of times this edge is taken=traversed, which equals the number of times its latch branch is "taken" rather than "falls-thru" (if it's true direction points to the header). Perhaps LoopEntryExitWeight or LoopInvocationWeight could be used instead/in addition.
> How so, given that OrigLoop has a single-entry, and an "expected" single-exit: there may be other "side/deopt" exits, but these are expected to have zero weight. E.g., when computing LoopHeaderWeight above, adding LoopEntryWeight (instead of LoopExitWeight) to BackEdgeTakenWeight seems more logical.

For infinite loop entry is 1 while exit is 0 :-). I understand this is extreme we will never meet but still....

> Strictly speaking, a BackEdge cannot exit: it is an edge going from a latch block to a header block, and "BackEdgeTaken" is the number of times this edge is taken=traversed, which equals the number of times its latch branch is "taken" rather than "falls-thru" (if it's true direction points to the header).

That's why I used TakenCount and FallThroughCount in the very first version what perfectly matches your description. I don't feel we are getting any better names with more iterations.... Perhaps LatchCycleWeight - number of times we go to loop header from the latch  and LatchExitWeight - number of times we go to loop exit from the latch?

> Perhaps LoopEntryExitWeight or LoopInvocationWeight could be used instead/in addition.
I think LoopEntryExitWeight may be confusing....
I think it makes sense to use EstimatedLoopInvocationWeight in conjunction with EstimatedTripCount as parameters to get/setEstimatedTripCount interface while LatchCycleWeight and LatchExitWeight in the implementation as they are little-bit more low level.





================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:680
 }
 
+static bool isLoopLatchControlsExit(Loop *L) {
----------------
Ayal wrote:
> Comment what this new function is for. Rename (see below)? Retain "Support loops ..." comment, added in D64553?
The comment is related to get/setLoopEstimatedTripCount and still there...


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:712
+  BranchInst *LatchBR =
+      dyn_cast<BranchInst>(L->getLoopLatch()->getTerminator());
+
----------------
Ayal wrote:
> dyn_cast >> cast
> 
> Perhaps update above function to do here something like
> `BranchInst *LatchBR = getExpectedExitLoopLatchBranch(L)`
> checking if it returned nullptr or not?
I was thinking about that in the first place but didn't come up with a "good" enough name. I can go with that name if you like :-)


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:733
+  uint64_t HeaderBlockWeight =
+      BackEdgeTakenWeight + BackEdgeExitWeight;
+  return llvm::divideNearest(HeaderBlockWeight, BackEdgeExitWeight);
----------------
Ayal wrote:
> Thanks for taking care of this fix to improve accuracy of estimated TC!
> But doing so deserves a separate patch, and tests. Note that it also effects loop unrolling, i.e., its effects are beyond LV. This part can be introduced either before or after the part that teaches LV to maintain profiling info.
Ok.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1089
+                                        Loop *RemainderLoop, uint64_t UF) {
+  assert(UnrolledLoop != RemainderLoop &&
+         "Unrolled and Remainder loops are expected to not be the same");
----------------
Ayal wrote:
> May also be worthwhile asserting that UF is positive (or greater than 1?)
I think we better support 1 which could be used in some corner cases....


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67905