[PATCH] D125987: [SLP] Account for cost of removing FMA opportunities by horizontal reduction

[Bill Schmidt via Phabricator via llvm-commits]

wjschmidt added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:9675
+// the tree of multiplies.
+static InstructionCost getFMALossCost(const TargetTransformInfo *TTI,
+                                      Value *TreeRootOp, FastMathFlags FMF,
----------------
ABataev wrote:
> RKSimon wrote:
> > wjschmidt wrote:
> > > ABataev wrote:
> > > > wjschmidt wrote:
> > > > > ABataev wrote:
> > > > > > I think it must be a part of TTI::getArithmeticReductionCost
> > > > > Because this will also be called in a case where it's not part of a reduction, I'd like to keep it separate.  (See my comment in the summary.)  Perhaps I shouldn't have split the patches so this would be more evident.
> > > > Anyway, maybe better to make it a part of TTI rather than put it to SLPVectorizer?
> > > That's certainly possible, and probably a good idea.  We'd need to pull out the final multiplication by Width and the SLP-specific debug stuff, but with that done it would fit nicely in TTI, and there might be potential for reuse.
> > Ideally the getArithmeticReductionCost costs wouldn't be affected but getArithmeticInstrCost could be adjusted to account for potential FMA patterns in the scalar/vector FADD costs
> Yep, good idea!
I'll look into possibilities.  That might be a bit awkward for the other use case.  It's clear that I should have combined the two patches to make this easier to judge, so I'll do that in the next revision.  Thanks for the ideas!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125987