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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

This is a good change. Thanks.

> Most checks should rarely trigger so I am using a 127:1 ratio.

Intuitively this feels reasonable, but I just don't feel confident enough.. like when `emitMemRuntimeChecks` is done, we really just don't know how likely these arrays overlap or not.

Is the plan to fill in something that's somewhat reasonable first which is basically a best effort guess, and then later fine tune that? This case is different from the general likely/unlikely weights, which is more straightforward.

Slightly different off-topic from the change itself, but for this to be future proof, I'm also wondering that eventually should we have the `BranchInst::Create` API for cond branch to always ask for branch weights if the parent function has non-zero entry count.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:395
 
+static constexpr uint32_t SCEVCheckBypassWeights[] = {1, 127};
+static constexpr uint32_t MemCheckBypassWeights[] = {1, 127};
----------------
Suggest add comments for these constants - what they are for and the reasoning for the selected weights.


================
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");
----------------
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.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159322