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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159322