[PATCH] D100745: [AArch64] Add AArch64TTIImpl::getMaskedMemoryOpCost function

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:1026
 
-  InstructionCost getGatherScatterOpCost(unsigned Opcode, Type *DataTy,
-                                         const Value *Ptr, bool VariableMask,
-                                         Align Alignment,
-                                         TTI::TargetCostKind CostKind,
-                                         const Instruction *I = nullptr) {
-    auto *VT = cast<FixedVectorType>(DataTy);
+  InstructionCost getMaskedMemOpCost(unsigned Opcode, Type *DataTy,
+                                     Align Alignment, bool VariableMask,
----------------
this name is really confusing, because I initially thought it was the same as getMaskedMemoryOpCost.

How about `getCommonMaskedMemoryOpCost`? And adding a comment that it is not the same as getMaskedMemoryOpCost. Perhaps you can also make this method `private`, as it's not supposed to be exposed outside this class.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:1031
     // Assume the target does not have support for gather/scatter operations
+    auto *VT = cast<FixedVectorType>(DataTy);
     // and provide a rough estimate.
----------------
How did this line move here?


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:1036
+    InstructionCost AddrExtractCost =
+        !IsGatherScatter
+            ? 0
----------------
perhaps just personal preference, but I think this reads easier:
  InstructionCost AddrExtractCost = IsGatherScatter ? AddrExtractCost = getVectorInstrCost(...) : 0;



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1043
+                                      TTI::TargetCostKind CostKind) {
+  if (!isa<ScalableVectorType>(Src))
+    return BaseT::getMaskedMemoryOpCost(Opcode, Src, Alignment, AddressSpace,
----------------
A cost of '2' would only be valid for legal types. It probably needs to consider legalisation, so that the cost of a <vscale x 4 x i64> would be 4.


================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:1441
   }
-  if (!isa<FixedVectorType>(Src))
-    return BaseT::getMaskedMemoryOpCost(Opcode, Src, Alignment, AddressSpace,
----------------
Why have you changed the ARM cost-model?


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

https://reviews.llvm.org/D100745