[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: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:
> 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.  
Sorry if this is confusing, the test here is whether the loop trip count is so small that we skip the unrolled loop and enter the epilogue immediatey. Trying to make the comment more clear now...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158642