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

[Evgeniy via Phabricator via llvm-commits]

ebrevnov marked 16 inline comments as done.
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:
> 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?


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1048
+  MDBuilder MDB(OrigLoopLatchBranch->getContext());
+  auto *UnrolledBBI = UnrolledLoop->getLoopLatch()->getTerminator();
+  bool IsTrueBackEdgeOrigLoop =
----------------
Ayal wrote:
> UnrolledBBI >> Unrolled[Loop]LatchBranch, as in OrigLoopLatchBranch.
> 
> As the names end up overflowing lines, can use Orig, Unrolled and Remainder to stand for OrigLoop, UnrolledLoop and RemainderLoop; i.e., taking "Loop" out.
Removed "Loop" from most names to make them a little shorter.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1064
+  const uint64_t OrigAverageTripCount =
+      OrigHeaderBlockWeight / OrigLoopEntryWeight;
+  // Calculate number of iterations in unrolled loop.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67905