[PATCH] D53011: [LV] Add support for vectorizing predicated strided accesses using masked interleave-group

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: include/llvm/Analysis/VectorUtils.h:131
+/// This function creates a shuffle mask for duplicating each of the elements
+/// in a vector of \p VF elements \p InterleaveFactor times. It can be used to
+/// transform a mask of \p VF elements into a mask of
----------------
"each of the elements in a vector of \p VF elements" >> “each element in a vector of \p VF elements”, or “each of the \p VF elements in a vector”.

Indicate “Interleaving” in the name of the method, given the InterleaveFactor argument?



================
Comment at: lib/Analysis/VectorUtils.cpp:822
+      // All members of a predicated interleave-group must have the same predicate,
+      // and currently must reside in the same BB.
+      if ((isPredicated(A->getParent()) || isPredicated(B->getParent())) &&
----------------
Below might look simpler using BlockA = A->getParent(); BlockB = B->getParent();


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:2598
     DL.getPointerSizeInBits() : ScalarTy->getPrimitiveSizeInBits();
-
   return ((DataWidth == 32 || DataWidth == 64) && ST->hasAVX()) ||
----------------
(Unrelated clang-format change)


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:416
+  /// Try to vectorize the interleaved access group that \p Instr belongs to,
+  ///  optionally masking the vector operations if \p BlockInMask is non-null.
+  void vectorizeInterleaveGroup(Instruction *Instr,
----------------
" optionally" >> "optionally"


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1985
+  assert ((!Group->isReverse() || !BlockInMask) && "Reversed Masked Interleave " 
+                                                   "group not supported. "); 
+
----------------
May be slightly better to check (!BlockInMask || !Group->isReverse()); or fold under the "if (IsMaskRequired)" below.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2041
+                   createTransposedMask(Builder, InterleaveFactor, VF),
+                   "interleaved.mask");
+        NewLoad = Builder.CreateMaskedLoad(NewPtrs[Part], Group->getAlignment(), 
----------------
Above may look simpler by first setting

```
  auto *Undefs = UndefValue::get(Mask->getType());
  auto *TransposedMask = createTransposedMask(Builder, InterleaveFactor, VF);

```


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2043
+        NewLoad = Builder.CreateMaskedLoad(NewPtrs[Part], Group->getAlignment(), 
+          ShuffledMask, UndefValue::get(VecTy), "wide.masked.load");
+      }
----------------
"wide.masked.load" >> "wide.masked.vec", to follow "wide.vec" more closely?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2120
+                   createTransposedMask(Builder, InterleaveFactor, VF),
+                   "interleaved.mask");
+      NewStoreInstr = Builder.CreateMaskedStore(IVec, NewPtrs[Part],
----------------
See above simplification comment.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4304
+             ? (TTI.isLegalMaskedLoad(Ty) || isLegalMaskedGather(Ty))
+             : (TTI.isLegalMaskedStore(Ty) || isLegalMaskedScatter(Ty));
+}
----------------
Better check only if `isLegalMaskedLoad` : `isLegalMaskedStore`. The purpose of optimizing interleaved accesses is to improve upon the use of Gather/Scatter.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7108
+    bool UseMaskedInterleaved = useMaskedInterleavedAccesses(*TTI);
+    IAI.analyzeInterleaving(UseMaskedInterleaved);
   }
----------------
Simpler to fold into

```
    IAI.analyzeInterleaving(useMaskedInterleavedAccesses(*TTI));
```


https://reviews.llvm.org/D53011