[PATCH] D93030: [AArch64][SVE]Add cost model for masked gather and scatter for scalable vector.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:925
 
+  /// \return The maximum value for vscale in scalable vectors such as
+  /// <vscale x 4 x i32>.
----------------
nit: The maximum value of `vscale` if the target specifies an architectural maximum vector length, and None otherwise.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:780
+      getMemoryOpCost(Opcode, VT->getElementType(), Alignment, 0, CostKind, I);
+  if (VF.isScalable()) {
+    Optional<unsigned> MaxNumVScale = getMaxVScale();
----------------
If you rewrite this in the opposite way, you can avoid indentation:

  if (!VF.isScalable())
    return BaseT::getGatherScatterOpCost(Opcode, DataTy, Ptr, VariableMask,
                                         Alignment, CostKind, I);



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:782
+    Optional<unsigned> MaxNumVScale = getMaxVScale();
+    if (MaxNumVScale.hasValue())
+      return MaxNumVScale.getValue() * VF.getKnownMinValue() * MemOpCost;
----------------
You can remove this `if` condition, if you just move the assert above this line.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:783
+    if (MaxNumVScale.hasValue())
+      return MaxNumVScale.getValue() * VF.getKnownMinValue() * MemOpCost;
+    // TODO: Replace assert for InstructionCost
----------------
nit: It's probably still worth asking how many gather operations this requires, e.g. for `TLI->getTypeLegalizationCost(DL, DataTy);`, and then multiply:

   auto LT = TLI->getTypeLegalizationCost(DL, DataTy);
   auto Cost = /*NumGathers*/ LT.first *
          /*NumElementsPerGather*/ (MaxNumVScale * LT.second.getElementCount.getKnownMinValue()) *
          MemOpCost;

Because it is multiplied, the result is currently the same, but perhaps there would be an added cost per gather instruction that is worth modelling at some point.



================
Comment at: llvm/test/Analysis/CostModel/AArch64/sve-getIntrinsicInstrCost-gather.ll:12
+; CHECK-LABEL: 'masked_gather_nxv4i32'
+; CHECK: Cost Model: Found an estimated cost of 64 for instruction:
+; CHECK-NEXT: Cost Model: Found an estimated cost of 0 for instruction:
----------------
Can you also check for the instruction (e.g. `llvm.masked.gather` and `ret`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93030