[PATCH] D147114: [LV] Use BFI to adjust cost of predicated instructions

[Evgeniy via Phabricator via llvm-commits]

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