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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:9668
 
+void SLPVectorizerPass::adjustForFMAs(InstructionCost &Cost,
+                                      ArrayRef<Value *> &VL) {
----------------
wjschmidt wrote:
> wjschmidt wrote:
> > RKSimon wrote:
> > > ABataev wrote:
> > > > Can this be hidden in the TTI completely anyhow? To avoid the extra calls of the adjustForFMAs completely in SLP.
> > > +1 - the only problem if that this will probably end up having to be added on a target-by-target basis in getArithmeticInstrCost - hopefully we can provide a helper wrapper so its only a one-liner in each target.
> > Hi -- sorry, I'll be out of town for the next week, so can't look at this right away.  In the meantime, can you please give me a little bit more idea exactly what you're looking for?  I've expressed my concerns about making direct changes to getArithmeticInstrCost for this in the comments for this revision -- can you please address those?  It's not at all clear to me what you're looking for other than "hide it from SLP somehow."
> I took a run at prototyping this yesterday for X86 TTI.  I put a shim in X86TTIImpl::getArithmeticInstrCost() to look for cases of FAdd and FSub that we expect to be folded into FMAs, and reduce their cost appropriately.  This was sufficient to prevent the horizontal reduction from happening, but as I expected, this doesn't help us with the tryToVectorizeList() case, where the cost modeling is only looking at the multiplies and not the add/sub fed by them.  (I.e., the second variant in the test case isn't fixed.)  I don't see any way to avoid explicit FMA checking in the SLP vectorizer to solve this, as explained in my summary for this revision.
> 
> Given this result, I'd like to proceed with the patch provided here.  It would be pleasant if we could hide the issue entirely from SLP, but it doesn't seem practical.  Thanks!
Could you provide more details why this does not help with tryToVectorizeList?


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

https://reviews.llvm.org/D125987