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

This is not answering the question, instead this is explaining how the pass works. 

> However I think this is the wrong place to explain how this pass works.

We do not need to explain how the pass works in order to explain why specific values are chosen. :) We can assume some knowledge of how the pass works from the readers.

> I have a feeling what you are really asking about here is some reasoning why "127" is a good value. 

Partially. But what I'm really asking is to put an explanation for the value you chose in the comment. Just saying this is generally unlikely, and it's a ball park estimate, not yet tuned can also be helpful (so people know they have more freedom to change it). 

I actually think we should put something like Hongtao's comment here.

> 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.


================
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};
----------------
MatzeB wrote:
> 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...
> This is about entering a loop epilogue, not the unrolled loop. 

For the unrolled loop, I was referring to the UnrolledLoopHeaderWeights above which shares the same value {1,127}. Sorry for causing confusion.

> And you are reading this correctly in that it should be a *low* probability.

No, I read it as it's a high probability to enter loop epilog. Given that we have `1 / UF` chance of loop trip count being exact multiples of `UF`, we enter loop epilog with a probability of `(UF - 1) / UF`. Am I missing something here? 

And btw, if I'm missing something obvious here, the answer doesn't need to go into comments.

> Though again this seems like the wrong place to explain the intricacies of this pass, it's just some constants...

I guess I care less where this is explained. But I think there needs to be an explanation for the values chosen somewhere. Currently there is no explanation.

However, given the constants are now extracted out, I think the most natural way is to have the explanation go along with the constants/values. 

We don't need explain the intricacies of this pass, but IMHO we need enough context for people to understand why these values were chosen, and how confident original author was about the chosen value. For such explanation, we can assume certain knowledge of the pass. Staring at some values and scratching our heads just isn't a good experience, so I'd err on the side of over-communicate when it comes to chosen constants.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158642