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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Thanks for making all the changes! More comments inline.



================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1040
+  uint64_t OrigBackedgeTakenWeight = 0;
+  uint64_t OrigLoopEntryWeight = 0;
+  auto *OrigLoopLatchBranch = OrigLoop->getLoopLatch()->getTerminator();
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1064
+  const uint64_t OrigAverageTripCount =
+      OrigHeaderBlockWeight / OrigLoopEntryWeight;
+  // Calculate number of iterations in unrolled loop.
----------------
ebrevnov wrote:
> Ayal wrote:
> > Note that this is rounding down. Can add half of the denominator to the nominator before dividing in order to round more accurately; this is what getLoopEstimatedTripCount() does, but it seems to be off by 1 as it computes BackEdgeTakenWeight / LoopEntryWeight rounded to nearest, instead of HeaderBlockWeight / LoopEntryWeight rounded to nearest...
> > 
> > Simply call OrigAverageTripCount = `getLoopEstimatedTripCount(OrigLoop)`?
> > 
> > Perhaps having a `setLoopEstimatedTripCount(Loop, EstimatedTripCount, EstimatedEntryWeight)` would help fold the identical treatment of UnrolledLoop and RemainderLoop into one function, which also takes care of figuring out the True/False vs. Backedge/Exit directions?
> This "off by 1" stopped me from using it in the first place since that could be important in some cases. OK, let's reuse getLoopEstimatedTripCount. To be able to do that there are some changes to getLoopEstimatedTripCount.
> This "off by 1" stopped me from using it in the first place since that could be important in some cases. OK, let's reuse getLoopEstimatedTripCount. To be able to do that there are some changes to getLoopEstimatedTripCount.

Indeed, best fix and reuse, in a dedicated patch as raised above, thereby isolating impact on such "some cases".


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


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:712
+  BranchInst *LatchBR =
+      dyn_cast<BranchInst>(L->getLoopLatch()->getTerminator());
+
----------------
dyn_cast >> cast

Perhaps update above function to do here something like
`BranchInst *LatchBR = getExpectedExitLoopLatchBranch(L)`
checking if it returned nullptr or not?


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:733
+  uint64_t HeaderBlockWeight =
+      BackEdgeTakenWeight + BackEdgeExitWeight;
+  return llvm::divideNearest(HeaderBlockWeight, BackEdgeExitWeight);
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:754
+  BasicBlock *LoopLatch = L->getLoopLatch();
+  BranchInst *LatchBranch = dyn_cast<BranchInst>(LoopLatch->getTerminator());
+  bool IsBackEdgeTakenOnTrue = L->contains(*succ_begin(LoopLatch));
----------------
ditto (dyn_cast >> cast, ...)


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:755
+  BranchInst *LatchBranch = dyn_cast<BranchInst>(LoopLatch->getTerminator());
+  bool IsBackEdgeTakenOnTrue = L->contains(*succ_begin(LoopLatch));
+
----------------
Better check similar to above `if (LatchBR->getSuccessor(0) != L->getHeader())`


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1085
+
+/// Set weights for the \p UnrolledLoop \p RemainderLoop based on weights for
+/// the \p OrigLoop.
----------------
"the \p UnrolledLoop \p RemainderLoop" >>
"\p UnrolledLoop and \p RemainderLoop"


================
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");
----------------
May also be worthwhile asserting that UF is positive (or greater than 1?)


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1090
+  assert(UnrolledLoop != RemainderLoop &&
+         "Unrolled and Remainder loops are expected to not be the same");
+
----------------
are expected to be distinct


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3472
+  // vector code caused by legality checks is not taken into account as unlikely
+  // case.
+  setProfileInfoAfterUnrolling(LI->getLoopFor(LoopScalarBody),
----------------
"is not taken into account as unlikely case" >>
"is ignored, assigning all the weight to the vector loop, optimistically"


================
Comment at: llvm/test/Transforms/LoopVectorize/check-prof-info.ll:6
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
Tests targeting x86 need to reside in LoopVectorize/X86


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67905