[PATCH] D111630: [LoopVectorize][CostModel] Update cost model for fmuladd intrinsic

[Rosie Sumpter via Phabricator via llvm-commits]

RosieSumpter added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:2177-2185
+  InstructionCost getFMulAddReductionCost(VectorType *Ty,
+                                          Optional<FastMathFlags> FMF,
+                                          TTI::TargetCostKind CostKind) {
+    InstructionCost FAddReductionCost = thisT()->getArithmeticReductionCost(
+        Instruction::FAdd, Ty, FMF, CostKind);
+    InstructionCost FMulCost =
+        thisT()->getArithmeticInstrCost(Instruction::FMul, Ty, CostKind);
----------------
paulwalker-arm wrote:
> Do we need a new TTI interface for this?  To my mind the costing side of TTI exists to cost real entities and in this instance the IR has no discrete concept for an FMA reduction.
> 
> Instead what we have is LoopVectorize pretending such a concept exists that is knows ahead of time it will simulate with separate fmul and ordered_fadd_reduce operations.  For this reason I think it would be better to explicitly cost that exact idiom within the same code that is using it.
> 
> So I guess I'm saying that if you move these three lines into LoopVectorize.cpp the patch can be much smaller and you're not creating a new interface for something that doesn't really exist.
Hi Paul, thanks for having a look at this. Initially I did put the cost calculation into the vectorizer, but there was some discussion (also a comment from @fhahn) about whether it would make more sense in ##TTI.getArithmeticReductionCost## and, to avoid changing the interface for ##getArithmeticReductionCost##, @david-arm and @sdesmalen suggested adding the new ##getFMulAddReductionCost##. I am happy to change it back to LoopVectorize.cpp if that seems like the better option.


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

https://reviews.llvm.org/D111630