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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:783
+    MDBuilder MDB(B.getContext());
+    BranchWeights = MDB.createBranchWeights(1, 127);
+  }
----------------
MatzeB wrote:
> 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...
> > 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.

I can see your way for the above, but here, the way I see it is that the choice of numbers and the intent behind them is not immediately obvious, so an API would make that clear. At minimum the values could be a constant that's reused? (maybe the `ArrayRef` overload of `createBranchWeights` would take that constant or something like that)

If this set of weights is this way elsewhere, doing a refactoring after makes sense, but (maybe I searched superficially) can't seem to find them other than in your recent patches on this topic, that's why I was thinking this may be the opportune moment - wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158642