[PATCH] D99719: [SLP] Better estimate cost of no-op extracts on target vectors.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3535
+                Cost -= TTI->getVectorInstrCost(Instruction::ExtractElement,
+                                                VecTy, I);
+            }
----------------
ABataev wrote:
> fhahn wrote:
> > ABataev wrote:
> > > fhahn wrote:
> > > > ABataev wrote:
> > > > > I think you need to use the real extract indices here to be more correct, i.e.
> > > > > ```
> > > > > for (unsigned I = Idx - EltsPerVector; I <= Idx; ++I)
> > > > > Cost -= TTI->getVectorInstrCost(Instruction::ExtractElement,
> > > > >                                                 cast<ExtractElementInst>(VL[I])->getVectorOperandType(), *getExtractIndex(cast<Instruction>(VL[I])));
> > > > > ```
> > > > Thanks, I think that's much better, updated.
> > > Hmm, I think more correct would be to do something like this:
> > > ```
> > > Cost += TTI->getShuffleCost(
> > >                   ShuffleKind.getValue(),
> > >                   FixedVectorType::get(VecTy->getElementType(), EltsPerVector), Mask);
> > > ```
> > > in case if `AllConsecutive` is `false` and ignore the initial shuffle cost completely. Also, you need to calculate the correct `Mask` here or pass `llvm::None`
> > > 
> > > So, I think you need to split it into separate parts.
> > > The first one for `*ShuffleKind == TargetTransformInfo::SK_PermuteSingleSrc` with improved shuffle cost for non-consecutive extracts and the generic one with the old functionality
> > That sounds good! I put up D99745. Was that what you had in mind for preparation? (Still need to add some comments, but I wanted to make sure that's what you actually had in mind)
> Sorry, I did not mean to split the patch into 2 parts :) I meant to split processing into 2 parts. Plus, the patch cannot be NFC since it changes the functionality.
Oh right! I think I see what you mean now. I move the new logic to a separate function and changed it to only add the shuffle costs for blocks that are not consecutive. I think the code should be much clearer now, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99719