[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:1119
+  if (LT.first > 1) {
+    auto *ValVTy = cast<VectorType>(Ty);
+    Type *VTy = VT.getTypeForEVT(ValVTy->getContext());
----------------
We already know `Ty` is a `VectorType` (see line 1104), so this cast is redundant.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1120
+    auto *ValVTy = cast<VectorType>(Ty);
+    Type *VTy = VT.getTypeForEVT(ValVTy->getContext());
+    unsigned CmpOpcode =
----------------
Nit: can you call this `LegalVTy`?

With the two comments above, that would become:

  Type *LegalVTy = EVT(LT.second).getTypeForEVT(Ty->getContext();


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1131-1138
+  int ISD;
+  if (Ty->isIntOrIntVectorTy()) {
+    ISD = IsUnsigned ? ISD::UMIN : ISD::SMIN;
+  } else {
+    assert(Ty->isFPOrFPVectorTy() &&
+           "Expected float point or integer vector type.");
+    ISD = ISD::FMINNUM;
----------------
The switch statement below and selection of the ISD seems unnecessary if the cost is the same, you can just write:

`return LegalizationCost + /*Cost of reduction*/ 2;`


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1159
+  if (LT.first > 1) {
+    auto *ValVTy = cast<VectorType>(ValTy);
+    Type *SingleOpTy = VT.getTypeForEVT(ValVTy->getContext());
----------------
We already know Ty is a VectorType (see line 1151), so this cast is redundant.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1191
+  if (isa<ScalableVectorType>(ValTy))
+    return getArithmeticReductionCostScalableVectorType(
+        Opcode, ValTy, IsPairwiseForm, CostKind);
----------------
nit: `s/getArithmeticReductionCostScalableVectorType/getArithmeticReductionCostSVE`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93639