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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1107
+                                                bool IsPairwise,
+                                                bool IsUnsigned,
+                                                TTI::TargetCostKind CostKind) {
----------------
My understanding is that the PairWise form is not something that can be expressed with the LLVM IR intrinsic and is therefore specific to fixed-width vectors which can use `shufflevector`.
Perhaps you can just assert IsPairWise is false. @david-arm any thoughts?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1110
+
+  if (!Ty->getElementCount().isScalable() &&
+      !CondTy->getElementCount().isScalable())
----------------
The scalable property should match for Ty and CondTy, so that can be an assert. With the if-condition changed to only check Ty.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1114
+                                         CostKind);
+  // TODO: Return invalid cost instead of assert when getMaxVScale is None
+  assert(getMaxVScale() && "Expected valid max vscale value");
----------------
remove TODO.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1117
+
+  unsigned MaxNumVScale = getMaxVScale().getValue();
+  auto LT = TLI->getTypeLegalizationCost(DL, Ty);
----------------
nit: `s/getMaxVScale().getValue()/*getMaxVScale()/`


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1122-1129
+  unsigned CmpOpcode;
+  if (Ty->isFPOrFPVectorTy()) {
+    CmpOpcode = Instruction::FCmp;
+  } else {
+    assert(Ty->isIntOrIntVectorTy() &&
+           "expecting floating point or integer type for min/max reduction");
+    CmpOpcode = Instruction::ICmp;
----------------
nit: `unsigned CmpOpcode = Ty->isFPOrFPVectorTy() ? Instruction::FCmp : Instruction::ICmp;`


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1135
+                         CmpInst::BAD_ICMP_PREDICATE, CostKind);
+  return LT.first * NumReduxLevels * MinMaxCost;
+}
----------------
I think this should be:
```Log2(LT.first) * MinMaxVectorCost + NumReduxLevels * MinMaxScalarCost```

Because the number of legalized vectors will do min/max operations amongst the vectors (to end up with a single vector), before it will do the min/max reduction on the elements within the vector.
That also means distinguishing the cost for CmpSel on vectors and CmpSel on scalars.


================
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");
----------------
remove TODO.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1148
+  unsigned MVTLen = MaxNumVScale * LVF.getKnownMinValue();
+  unsigned NumReduxLevels = Log2_32(MVTLen);
+  unsigned ArithCost = getArithmeticInstrCost(Opcode, ValTy, CostKind);
----------------
nit: You can just as well inline this into the expression below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93639