[PATCH] D35829: [X86][LLVM]Expanding Supports lowerInterleavedStore() in X86InterleavedAccess (VF16 stride 4).

[Guy Blank via Phabricator via llvm-commits]

guyblank added inline comments.


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:113
+  // 2. 16-element vectors of 8 bits on AVX.
+  // 3. 32-element vectors of 8 bits on AVX.
   if (isa<LoadInst>(Inst)) {
----------------
the comment is a bit misleading since it depends on whether we are loading or storing. can you expand the comment to state exactly what is supported? 


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:126
 
-  if (!Subtarget.hasAVX() || Factor != 4 ||
-      (ShuffleElemSize != 64 && ShuffleElemSize != 8) ||
+  if (DL.getTypeSizeInBits(ShuffleEltTy) == 8 && isa<StoreInst>(Inst) &&
+      (WideInstSize == 512 || WideInstSize == 1024) && Subtarget.hasAVX() &&
----------------
you can use ShuffleElemSize 


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:127
+  if (DL.getTypeSizeInBits(ShuffleEltTy) == 8 && isa<StoreInst>(Inst) &&
+      (WideInstSize == 512 || WideInstSize == 1024) && Subtarget.hasAVX() &&
+      Factor == 4)
----------------
you can early exit at the beginning of the function on !Subtarget.hasAVX() 


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:132
+  if (!Subtarget.hasAVX() || Factor != 4 || ShuffleElemSize != 64 ||
       WideInstSize != (Factor * ShuffleElemSize * SupportedNumElem))
     return false;
----------------
so this check isn't needed in the previous if?

Can you try to simplify this function? its getting overly complicated IMO.


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:366
     break;
+  case 16:
+    interleave8bitStride4(DecomposedVectors, TransposedVectors, MVT::v16i8,
----------------
maybe pass NumSubVecElems to interleave8bitStride4 and have it calculate the MVTs ?


https://reviews.llvm.org/D35829