[PATCH] D15677: Cost for Gather and Scatter operations.

[Ayal Zaks via llvm-commits]

Ayal added inline comments.

================
Comment at: ../include/llvm/Analysis/TargetTransformInfo.h:465
@@ +464,3 @@
+  /// \p Ptr - pointer [or vector of pointers] - address[es] in memory
+  /// \p VariableMask - true when the memory access is predicated
+  /// \p Alignment - alignment of single element
----------------
... with a mask that is not a compile-time constant

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:572
@@ -571,2 +571,3 @@
     { ISD::SIGN_EXTEND, MVT::v8i64,  MVT::v8i16,  1 },
+    { ISD::SIGN_EXTEND, MVT::v8i64,  MVT::v16i32, 3 },
 
----------------
this is independent of the rest of the patch, right?

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:1310
@@ +1309,3 @@
+  // to 32. It is essential for VF 16. If the index can't be reduced to 32, the
+  // operation 16 x 64 does not fit in zmm and should be split.
+  auto getIndexSizeInBits = [](Value *Ptr, const DataLayout& DL) {
----------------
// operation will use 16 x 64 indices which do not fit in a zmm and needs to split. Also check that the base pointer is the same for all lanes, and that there's at most one variable index.

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:1314
@@ +1313,3 @@
+    GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(Ptr);
+    if (IndexSize < 64 && !GEP)
+      return IndexSize;
----------------
this should be "if (IndexSize < 64 || !GEP)" to reverse your original condition.

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:1338-1339
@@ +1337,4 @@
+  // By default the IndexSize is equal to pointer size.
+  unsigned IndexSize = (VF >= 16) ? getIndexSizeInBits(Ptr, DL) :
+    DL.getPointerSizeInBits();
+
----------------
Alternatively, pass VF to getIndexSizeInBits and fold this logic of checking for VF < 16 in it?

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:1353-1354
@@ +1352,4 @@
+
+  // The gather / scatter cost is given by Intel architects. It is a rough
+  // number since we are looking at one intruction in a time.
+  const int GSOverhead = 2;
----------------
in[s]truction at a time

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:1358
@@ +1357,3 @@
+                                           Alignment, AddressSpace);
+
+}
----------------
no need for empty line

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:1365
@@ +1364,3 @@
+/// SrcVTy - The type of the data vector that should be gathered or scattered.
+/// Masked - The operation has non-constant mask.
+/// Alignment - Alignment for one element.
----------------
VariableMask

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:1425
@@ +1424,3 @@
+  // the mask vector will add more instructions. Right now we decide
+  // to scalarize vector-4 for KNL.
+  if (VF == 2 || (VF == 4 && !ST->hasVLX()))
----------------
Mark as TODO?

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.h:80
@@ -80,1 +79,3 @@
+  int getGatherScatterOpCost(unsigned Opcode, Type *DataTy, Value *Ptr,
+                             bool Masked, unsigned Alignment);
   int getAddressComputationCost(Type *PtrTy, bool IsComplex);
----------------
VariableMask


Repository:
  rL LLVM

http://reviews.llvm.org/D15677