[PATCH] D111668: [LoopPredication] Calculate profitability without BPI
Evgeniy via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 19 22:30:08 PDT 2021
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
More information about the llvm-commits
mailing list