[PATCH] D159322: LoopVectorize: Set branch_weight for conditional branches

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.

lgtm assuming comments for constants will be added. thanks.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:395
 
+static constexpr uint32_t SCEVCheckBypassWeights[] = {1, 127};
+static constexpr uint32_t MemCheckBypassWeights[] = {1, 127};
----------------
wenlei wrote:
> Suggest add comments for these constants - what they are for and the reasoning for the selected weights.
We discussed this topic on D158642, and it applies here too :) 


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:400
+
+static void setBranchWeights(BranchInst &BI, ArrayRef<uint32_t> Weights) {
+  assert(BI.getNumSuccessors() == 2 && "expected conditional branch");
----------------
MatzeB wrote:
> MatzeB wrote:
> > wenlei wrote:
> > > nit: this is not really unique to loop vectorize. Either move it to common header, and use it consistently across the code base, or stick with the current convention, which achieves the same thing with this:
> > > 
> > > ```
> > > Br->setMetadata(LLVMContext::MD_prof, MDBuilder(Br->getContext())
> > >                          .createBranchWeights(TrueWeight, FalseWeight));
> > > ```
> > > 
> > > I'd avoid inconsistency if possible.  
> > Well honestly I did not want to create a helper function, because I knew reviewers would ask me to apply to the whole project. I also tried applying it to the whole project, but there are a lot of varying patterns in the codebase that I would regress when forcing them to this API:
> > * Some cases construct the weights node upfront and re-use it within a loop.
> > * Some instances already have `MDBuilder` or `Context` references around and want to re-use them.
> > * Some instances don't have `BI` added to the basic block yet so `BI.getContext()` call will fail.
> > * Some instances rather use the `IRBuilder::createCondBr` etc.
> > 
> > So I did not feel like I created consistency but just added to the amount of inconsistency by adding one more helper. Hence I kept this local...
> And for the record: There may be value in changing bigger parts of the codebase, but it may be bigger than it looks and is certainly out of scope for this change.
> There may be value in changing bigger parts of the codebase, but it may be bigger than it looks and is certainly out of scope for this change.

Agreed, and yes that should be a separate change.  

Not a deal breaker, but I'd prefer defer all this to a bigger change for the codebase later, and just use the one liner here..

> So I did not feel like I created consistency but just added to the amount of inconsistency by adding one more helper. Hence I kept this local...

The thing is.. later people may add another static/local setBranchWeights to another file, because they look at this change and be like - oh, there's precedence of doing that and I didn't create that inconsistency.. :) 

If you really feel strongly about keeping this local helper, I'm fine.. But I suggest just use that one-liner without the helper. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159322