[PATCH] D93639: [AArch64][SVE]Add cost model for vector reduce for scalable vector

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added a comment.

Have a couple nits, but seems reasonable to me. Somebody else should review as well.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1118
+  unsigned MaxNumVScale = getMaxVScale().getValue();
+  auto LT = TLI->getTypeLegalizationCost(DL, Ty);
+  ElementCount LVF = LT.second.getVectorElementCount();
----------------
please name the type


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1139
+int AArch64TTIImpl::getArithmeticReductionCostSVE(
+    unsigned Opcode, VectorType *ValTy, bool IsPairwise,
+    TTI::TargetCostKind CostKind) {
----------------
ScalableVectorType


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1141
+    TTI::TargetCostKind CostKind) {
+  // TODO: Return invalid cost instead of assert when getMaxVScale is None
+  assert(getMaxVScale() && "Expected valid max vscale value");
----------------
sdesmalen wrote:
> remove TODO.
why not just return invalid cost now and assert at the call site?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1159
+  ElementCount VF = ValTy->getElementCount();
+  if (VF.isScalable())
+    return getArithmeticReductionCostSVE(Opcode, ValTy, IsPairwiseForm,
----------------
NIT:  `isa<ScalableVectorType>(ValTy)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93639