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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp:783
+    MDBuilder MDB(B.getContext());
+    BranchWeights = MDB.createBranchWeights(1, 127);
+  }
----------------
hoy wrote:
> mtrofin wrote:
> > 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?
> > 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...
> 
> The old preheader can be just some block dominating the loop header, for the sake of block execution count.
> 
> 
> Right, the branch is comparing trip count per each loop entry with the unroll factor. But to simulate that probability at compile time, I feel we can use the aggregated loop trip count (i.e., profile count) divided by the preheader count to compare with the unroll factor. Let me know if this feels wrong.
> 
> 
> 
> 
Talked offline. The average trip count is already used to check if loop unroll is beneficial. Here we are pretty sure the unrolled loop body is going to be run, so giving a 99.9...% ratio is reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158642