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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:779
   // Branch to either remainder (extra iterations) loop or unrolling loop.
-  B.CreateCondBr(BranchVal, RemainderLoop, UnrollingLoop);
+  MDNode *BranchWeights = nullptr;
+  if (hasBranchWeightMD(*Latch->getTerminator())) {
----------------
mtrofin wrote:
> should this logic be factored in a `getBranchWeights(Latch, Count)` - something like that?
and put it where? I find that having the branch weight decisions right next to the code creating the branch helps readability of the code as you don't have to jump around.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:783
+    MDBuilder MDB(B.getContext());
+    BranchWeights = MDB.createBranchWeights(1, 127);
+  }
----------------
hoy wrote:
> mtrofin wrote:
> > one thought about the 1:127 ratio: how about we had a `MDBuilder::createUnrolledLoopSkipWeights()` doing the setting - more readable, API captures the intent, etc?
> Can this be computed based on the loop preheader count and the main loop trip count? IIUC, the block execution count of the preheader indicates how many times the loop is entered from outside the loop. Thus that the loop trip count divided by the preheader count indicates an average trip count per loop execution. If that is greater than the unrolling factor, then the unrolled loop should be always executed?
> 
> 
> 
> how about we had a `MDBuilder::createUnrolledLoopSkipWeights()`

As above I think this hurts readability as it moves the logic away into another file. Also if we decide this being the preferred method, then I'd rather see a mass-refactoring of relevant places rather than starting with one-offs in diffs.

> Can this be computed based on the loop preheader count and the main loop trip count?

I don't see how. The old preheader (if it existed at all) will give you the ratio of zero and non-zero trip counts. But this here needs to catch the cases of trip-counts being smaller than the unroll factor...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158642