[PATCH] D111668: [LoopPredication] Calculate profitability without BPI

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopPredication.cpp:971
+
+  auto ComputeBranchProbability =
+      [&](const BasicBlock *ExitingBlock,
----------------
anna wrote:
> ebrevnov wrote:
> > This looks like a natural generalization of "bool Instruction::extractProfMetadata(uint64_t &TrueVal, uint64_t &FalseVal) const". Why don't we extend existing implementation of "extractProfMetadata" to handle more than 2 operands and overloaded existing API with "bool Instruction::extractProfMetadata(SmallVectorImpl<uint64_t> &) const"?
> > 
> > PS: Looks tempting to place total weight as a first element in the resulting vector. WDYT?
> The `SmallVector` won't buy us what we need. After retrieving those individual profile data, we still need to calculate the `Numerator` by checking ` if (Term->getSuccessor(i) == ExitBlock)`, i.e. what we do at line 983.
> 
> Also, btw, we have `Instruction::extractProfTotalWeight`, which computes what I have in `Denominator`, but we anyway need to compute the `Numerator` so not sure if it's worth it.
> This looks like a natural generalization of "bool Instruction::extractProfMetadata(uint64_t &TrueVal, uint64_t &FalseVal) const". Why don't we extend existing implementation of "extractProfMetadata" to handle more than 2 operands and overloaded existing API with "bool Instruction::extractProfMetadata(SmallVectorImpl<uint64_t> &) const"?
> 
> PS: Looks tempting to place total weight as a first element in the resulting vector. WDYT?




================
Comment at: llvm/lib/Transforms/Scalar/LoopPredication.cpp:971
+
+  auto ComputeBranchProbability =
+      [&](const BasicBlock *ExitingBlock,
----------------
ebrevnov wrote:
> anna wrote:
> > ebrevnov wrote:
> > > This looks like a natural generalization of "bool Instruction::extractProfMetadata(uint64_t &TrueVal, uint64_t &FalseVal) const". Why don't we extend existing implementation of "extractProfMetadata" to handle more than 2 operands and overloaded existing API with "bool Instruction::extractProfMetadata(SmallVectorImpl<uint64_t> &) const"?
> > > 
> > > PS: Looks tempting to place total weight as a first element in the resulting vector. WDYT?
> > The `SmallVector` won't buy us what we need. After retrieving those individual profile data, we still need to calculate the `Numerator` by checking ` if (Term->getSuccessor(i) == ExitBlock)`, i.e. what we do at line 983.
> > 
> > Also, btw, we have `Instruction::extractProfTotalWeight`, which computes what I have in `Denominator`, but we anyway need to compute the `Numerator` so not sure if it's worth it.
> > This looks like a natural generalization of "bool Instruction::extractProfMetadata(uint64_t &TrueVal, uint64_t &FalseVal) const". Why don't we extend existing implementation of "extractProfMetadata" to handle more than 2 operands and overloaded existing API with "bool Instruction::extractProfMetadata(SmallVectorImpl<uint64_t> &) const"?
> > 
> > PS: Looks tempting to place total weight as a first element in the resulting vector. WDYT?
> 
> 
> The `SmallVector` won't buy us what we need. After retrieving those individual profile data, we still need to calculate the `Numerator` by checking ` if (Term->getSuccessor(i) == ExitBlock)`, i.e. what we do at line 983.
Sure, we will still need to iterate other the vector to extract required weight. But all the code responsible for parsing "branch_weight" metadata will move to a single place. If representation of "branch_weight" metadata is changed one day there will be a single place to adjust.

> 
> Also, btw, we have `Instruction::extractProfTotalWeight`, which computes what I have in `Denominator`, but we anyway need to compute the `Numerator` so not sure if it's worth it.
I don't see reasons to call `Instruction::extractProfTotalWeight` in this case either. My proposal was to include total weight as the first element of the vector populated by "bool Instruction::extractProfMetadata(SmallVectorImpl<uint64_t> &) const". The motivation is simple, the weight itself is mostly useless because it is relative to total weight. Thus we always need total weight as well.



================
Comment at: llvm/lib/Transforms/Scalar/LoopPredication.cpp:977
+    unsigned NumSucc = Term->getNumSuccessors();
+    if (isValidProfileData(ProfileData, Term)) {
+      uint64_t Numerator = 0, Denominator = 0, ProfVal = 0;
----------------
anna wrote:
> ebrevnov wrote:
> > Looks like we already check that there is a "valid" profile at line 968. Why do it again?
> Because the first check is for Latch terminators, while this one is for all branches exiting the loop. See usage of `ComputeBranchProbability` below.
Makes sense. Overlooked second call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111668