[PATCH] D105432: [Analysis] Add simple cost model for strict (in-order) reductions

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for the changes @david-arm. Just left a few more nits.



================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:1152-1154
+    if (FMF != None && !(*FMF).allowReassoc())
+      return true;
+    return false;
----------------
nit: `return FMF && !(*FMF).allowReassoc());`


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:1160
   /// This is the cost of reducing the vector value of type \p Ty to a scalar
-  /// value using the operation denoted by \p Opcode.
+  /// value using the operation denoted by \p Opcode. The \p Opcode and
+  /// FastMathFlags parameter \p FMF together indicate what type of
----------------
I think the opcode is no longer what decides what the type of the reduction is. It is mostly the type and the FMF.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:2082
+                                          TTI::TargetCostKind CostKind) {
+    if (isa<ScalableVectorType>(Ty))
+      return InstructionCost::getInvalid();
----------------
nit: please add a comment saying that targets must implement a default for the scalable case, because we can't know how many lanes the vector has.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:2084
+      return InstructionCost::getInvalid();
+    auto *VTy = cast<FixedVectorType>(Ty);
+    InstructionCost ExtractCost =
----------------
nit: add empty line above cast<FixedVectorType>


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1548
       getMemoryOpCost(Opcode, VT->getElementType(), Alignment, 0, CostKind, I);
-  unsigned MaxNumElementsPerGather =
-      MaxNumVScale.getValue() * LegalVF.getKnownMinValue();
-  return LT.first * MaxNumElementsPerGather * MemOpCost;
+  return LT.first * MemOpCost * getMaxNumElements(LegalVF);
 }
----------------
Why are you changing the GatherScatterOpCost in this patch?


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

https://reviews.llvm.org/D105432