[PATCH] D158642: LoopUnrollRuntime: Add weights to all branches

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:61
+// we do not enter the unrolled loop at all.
+static const uint32_t UnrolledLoopHeaderWeights[] = {1, 127};
+// Probability that the epilogue loop will be executed at all.
----------------
nit: in the comment also explain why it's unlikely for loop to not enter unrolled version at all. Like does the unroll only happen with known trip count (which is bigger than unroll factor)? 


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:62
+static const uint32_t UnrolledLoopHeaderWeights[] = {1, 127};
+// Probability that the epilogue loop will be executed at all.
+static const uint32_t EpilogHeaderWeights[] = {1, 127};
----------------
It's good to have constants extracted out, but when we do that, we can try to make it meaningful when someone just look at the constants without looking at the uses. 

If {1,127} means *low* probability for not entering unrolled loop at all, it'd be confusing for the same {1,127} to also mean *high* probability for epilog loop to be executed. 

Easiest way to fix is to add comment for each individual weight.. 


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:310
+  if (hasBranchWeightMD(*Latch->getTerminator())) {
+    // Assume equal distribution in interval [0;Count).
+    MDBuilder MDB(B.getContext());
----------------
nit: you mean `[0, count)`? 


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:399-402
+        // Assign the maximum possible trip count as the back edge weight for
+        // the remainder loop if the original loop comes with a branch weight.
+        assert(Count > 1 && "unroll factor must be 2 or more");
+        uint64_t BackEdgeWeight = (Count - 1) * OrigExitWeight;
----------------
I know this is just refactoring from `updateLatchBranchWeightsForRemainderLoop`, but I'm wondering if the assumption is reasonable? If we assume linear distribution for loop trip counts, then the average trip count for the remainder should be `Count / 2`, rather than `Count - 1`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158642