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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm 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);
----------------
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.


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

https://reviews.llvm.org/D111630