[PATCH] D36960: [X86][LLVM]Expanding Supports lowerInterleavedLoad() in X86InterleavedAccess (VF{8|16|32} stride 3).

[Guy Blank via Phabricator via llvm-commits]

guyblank added inline comments.


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:110
 
   // Currently, lowering is supported for the following vectors with stride 4:
   // 1. Store and load of 4-element vectors of 64 bits on AVX.
----------------
update comment please


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:130
 
+  if (ShuffleElemSize == 8 && isa<LoadInst>(Inst) &&
+      (WideInstSize == 192 || WideInstSize == 384 || WideInstSize == 768))
----------------
should this be just for factor == 3 ?
if so, should the code above also change to be only for factor == 4 ?


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:170
+  unsigned int NumLoads = NumSubVectors;
+  if (DL.getTypeSizeInBits(VecTy) == 768) {
+    Type *VecTran =
----------------
can you explain what's happening here?


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:373
+//  createAlignMask returns the shuffle mask of vpalign instruction.
+//  Vpaling works according to lanes
+//  Where Lane is the # of lanes in a register:
----------------
typo: vpaling -> vpalign


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:378
+//  For Lane = 2 shuffle pattern is {DiffToJump,...,VF/2-1,VF/2,...,VF-1}.
+//  For Lane = 1 shuffle pattern is {DiffToJump,...,VF}.
+//  The  VF argument stands for vector factor.
----------------
should be {DiffToJump, ..., DiffToJump + VF - 1} ?


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:384
+//  VectorSize contains the number of bits.
+//  AlignBegin is a booleand that indecat the diercation of the aligment
+//  (false - align to the "rigth" side while true - align to the "rigth" side)
----------------
typos, should be:
AlignBegin is a boolean that indicates the direction of the alignment


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:385
+//  AlignBegin is a booleand that indecat the diercation of the aligment
+//  (false - align to the "rigth" side while true - align to the "rigth" side)
+static void createAlignMask(int VF, int Align, SmallVectorImpl<uint32_t> &Mask,
----------------
typo: rigth -> right
plus, on of these should be "left"
and maybe something ilke AlignDirection would be a better name?


https://reviews.llvm.org/D36960