[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: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.
----------------
wenlei wrote:
> 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)? 
> 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)?

The header deals with the amount of iterations not fitting into the unroll factor. i.e. if the loop requires 43 iterations and you have an unroll factor of 4, then the header will do 3 iterations and we enter the unrolled loop to process the remaining 40 iterations (that are now 10 unrolled ones).

However I think this is the wrong place to explain how this pass works. It's just a constant used in the algorithm. I'd rather not try to write explanations of how the algorithm works here, we should have that at the beginning of the file, in the functions performing the transformations and policy questions you hint at are in `LoopUnrollPass.cpp` this file here is mostly about mechanisms.

I have a feeling what you are really asking about here is some reasoning why "127" is a good value. And honestly I don't have a deeper analysis behind this, except for looking at some small toy examples. This should be clearly better than the default 50:50 split we end up when not adding anything, so I hope we can leave tuning for later (if necessary).


================
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};
----------------
wenlei wrote:
> 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.. 
> 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.

This is about **entering a loop epilogue**, not the unrolled loop. And you are reading this correctly in that it should be a *low* probability.

The comment already states this. The code either produces a prologue or an epilogue to deal with the extra iterations that don't fit the unrolled loop. Though again this seems like the wrong place to explain the intricacies of this pass, it's just some constants...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158642