[PATCH] D147114: [LV] Use BFI to adjust cost of predicated instructions
Evgeniy via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 30 01:35:18 PDT 2023
ebrevnov marked 3 inline comments as done.
ebrevnov added a comment.
In D147114#4230437 <https://reviews.llvm.org/D147114#4230437>, @dmgreen wrote:
>
Do you have any benchmark data to back it up?
I have motivating example which shows almost 2x degradation due to vectorization. I will add it as lit test.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4538
// Scale the cost by the probability of executing the predicated blocks.
// This assumes the predicated block for each vector lane is equally
// likely.
----------------
fhahn wrote:
> this needs updating now I think
I just removed second sentence. I think everything is clear without extra details.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5596
+ InstructionCost &Cost, const BasicBlock *BB) const {
+ assert(Cost.isValid() && "Can't scale invalid cost");
+
----------------
fhahn wrote:
> dmgreen wrote:
> > Could invalid cost handling be pushed into this function. It would help not needing it at all the call sites.
> Yeah I think that would help readability
Fixed.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5602
+ auto HeaderFreq = BFI->getBlockFreq(TheLoop->getHeader()).getFrequency();
+ if (HeaderFreq == 0)
+ return Cost;
----------------
fhahn wrote:
> Could you explain the reasoning here? Maybe fall back to the original code using `getReciprocalPredBlockProb` here?
Good idea. Fixed as suggested.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10586
BlockFrequencyInfo *BFI = nullptr;
- if (PSI && PSI->hasProfileSummary())
+ //if (PSI && PSI->hasProfileSummary())
BFI = &AM.getResult<BlockFrequencyAnalysis>(F);
----------------
dmgreen wrote:
> Is BFI intended to be required now? Some of the other code makes it look like it is still optional.
> Is BFI intended to be required now?
Yes. In fact, it should look like:
BlockFrequencyInfo *BFI = &AM.getResult<BlockFrequencyAnalysis>(F);
Will fix.
> Some of the other code makes it look like it is still optional.
Right. Before this change there was exactly one use of BFI guarded by PSI. I would suggest not to touch this place (at least now) because if compile time regression shows up we may want to conditionally request BFI for predicated loops only.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147114/new/
https://reviews.llvm.org/D147114
More information about the llvm-commits
mailing list