[PATCH] D15690: Gather and Scatter intrinsics in the Loop Vectorizer

[Ayal Zaks via llvm-commits]

Ayal added inline comments.

================
Comment at: ../include/llvm/IR/IRBuilder.h:436
@@ +435,3 @@
+  /// \brief Create a call to Masked Gather intrinsic
+  CallInst *CreateMaskedGather(Value *Ptrs, unsigned Align, Value *Mask,
+                               Value *PassThru = 0, const Twine& Name = "");
----------------
Set default Mask to 0 similar to scatter below?

================
Comment at: ../lib/IR/IRBuilder.cpp:250
@@ -249,1 +249,3 @@
 
+/// Create a call to a Masked Load intrinsic.
+/// Ptrs     - the vector of pointers for loading
----------------
Load >> Gather

================
Comment at: ../lib/IR/IRBuilder.cpp:253
@@ +252,3 @@
+/// Align    - alignment for one element
+/// Mask     - an vector of booleans which indicates what vector lanes should
+///            be accessed in memory
----------------
a[n] vector

================
Comment at: ../lib/IR/IRBuilder.cpp:265
@@ +264,3 @@
+
+  PointerType *PtrTy = cast<PointerType>(PtrsTy->getElementType());
+  unsigned NumElts = PtrsTy->getVectorNumElements();
----------------
As Sanjay suggested, you could do
  auto *PtrsTy = cast<...;
  auto *PtrTy = cast<...;
and rely on these casts asserting for you. Also good to place these two lookalike variables together.

================
Comment at: ../lib/IR/IRBuilder.cpp:278
@@ +277,3 @@
+/// Create a call to a Masked Scatter intrinsic.
+/// Val   - the data to be stored,
+/// Ptrs  - the vector of pointers, where the Val elements should be stored
----------------
Val >> Data

================
Comment at: ../lib/IR/IRBuilder.cpp:279
@@ +278,3 @@
+/// Val   - the data to be stored,
+/// Ptrs  - the vector of pointers, where the Val elements should be stored
+/// Align - alignment for one element
----------------
ditto

================
Comment at: ../lib/IR/IRBuilder.cpp:281
@@ +280,3 @@
+/// Align - alignment for one element
+/// Mask  - an vector of booleans which indicates what vector lanes should
+///         be accessed in memory
----------------
a[n] vector

================
Comment at: ../lib/IR/IRBuilder.cpp:298
@@ +297,3 @@
+         PtrTy->getElementType() ==  DataTy->getElementType() &&
+        "Incopatible pointer and data types");
+
----------------
Inco[m]patible.
Two separate asserts?

================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:2401-2402
@@ -2385,3 +2400,4 @@
   bool Reverse = ConsecutiveStride < 0;
-  bool UniformLoad = LI && Legal->isUniform(Ptr);
-  if (!ConsecutiveStride || UniformLoad)
+  bool CreateGatherScatter = false;
+  if (!ConsecutiveStride)
+    CreateGatherScatter =
----------------
Can fold into
  bool CreateGatherScatter = (!ConsecutiveStride && ((LI && ...) || (SI && ...)));



================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:2470
@@ +2469,3 @@
+      assert(CreateGatherScatter  &&  "The instruction should be scalarized");
+      if (Gep) {
+        SmallVector<VectorParts, 4> OpsV;
----------------
spatel wrote:
> The function is getting too long / indented. I would prefer to see it broken up with helper functions.
comment that in vectorizing Gep, across UF parts, we want to keep each loop-invariant base or index of Gep scalar.

================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:2483
@@ +2482,3 @@
+            Ops.push_back(OpsV[i][Part]);
+          Value *GEPBasePtr = OpsV[0][Part];
+          Value *NewGep = Builder.CreateGEP(nullptr, GEPBasePtr, Ops,
----------------
looks better to first set GEPBasePtr and then set the GEP Ops.

================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:5331
@@ -5264,1 +5330,3 @@
 
+/// \brief Check if the load/store instruction may be transfered to
+/// gather/scatter during vectorization.
----------------
spatel wrote:
> typo: transferred
transfer[r]ed to >> translated into

================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:5599
@@ -5514,1 +5598,3 @@
+
+    // Wide load/stores.
     if (Legal->isMaskRequired(I))
----------------
can assert ConsecutiveStride here to be sure


http://reviews.llvm.org/D15690