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

[Sanjay Patel via llvm-commits]

spatel added a comment.

I don't know the vectorizer or scatter/gather well enough for a complete review, so I've just pointed out some small fixes.

Also, I think it's fine to have scatter and gather in one patch for review, but they should be split into separate patches for commit to make it easier to investigate bugs/regressions.


================
Comment at: ../lib/IR/IRBuilder.cpp:261-263
@@ +260,5 @@
+                                            const Twine& Name) {
+  auto *PtrsTy = dyn_cast<VectorType>(Ptrs->getType());
+  assert(PtrsTy && PtrsTy->getElementType()->isPointerTy() &&
+    "Expected vector of pointers for gather");
+
----------------
'dyn_cast' can be a plain 'cast' since we're asserting.

================
Comment at: ../lib/IR/IRBuilder.cpp:265
@@ +264,3 @@
+
+  PointerType *PtrTy = cast<PointerType>(PtrsTy->getElementType());
+  unsigned NumElts = PtrsTy->getVectorNumElements();
----------------
This can be 'auto' to match the earlier cast.

================
Comment at: ../lib/IR/IRBuilder.cpp:285-286
@@ +284,4 @@
+                                             unsigned Align, Value *Mask) {
+  auto *PtrsTy = dyn_cast<VectorType>(Ptrs->getType());
+  auto *DataTy = dyn_cast<VectorType>(Data->getType());
+
----------------
Use plain 'cast' since we're asserting.

================
Comment at: ../lib/IR/IRBuilder.cpp:293
@@ +292,3 @@
+
+  PointerType *PtrTy = cast<PointerType>(PtrsTy->getElementType());
+  unsigned NumElts = PtrsTy->getVectorNumElements();
----------------
Use 'auto' for consistency.

================
Comment at: ../lib/IR/IRBuilder.cpp:304
@@ +303,3 @@
+  Value * Ops[] = {Data, Ptrs, getInt32(Align), Mask};
+  // Type of the data to be stored - the only one overloaded type
+  return CreateMaskedIntrinsic(Intrinsic::masked_scatter, Ops, DataTy);
----------------
I don't understand the comment. Please explain more or delete.

================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:2397
@@ +2396,3 @@
+
+  // If the pointer is non-consecutive and gather/scatter is not suppored
+  // scalarize the instruction.
----------------
typo: supported

================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:2468
@@ +2467,3 @@
+    } else {
+      // Supported random access (gather / scatter)
+      assert(CreateGatherScatter  &&  "The instruction should be scalarized");
----------------
Use complete sentence for comment.

================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:2469
@@ +2468,3 @@
+      // Supported random access (gather / scatter)
+      assert(CreateGatherScatter  &&  "The instruction should be scalarized");
+      if (Gep) {
----------------
Extra spaces.

================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:2470-2493
@@ -2437,10 +2469,26 @@
+      assert(CreateGatherScatter  &&  "The instruction should be scalarized");
+      if (Gep) {
+        SmallVector<VectorParts, 4> OpsV;
+        for (Value *Op : Gep->operands()) {
+          if (PSE.getSE()->isLoopInvariant(PSE.getSCEV(Op), OrigLoop))
+            OpsV.push_back(VectorParts(UF, Op));
+          else
+            OpsV.push_back(getVectorValue(Op));
+        }
+
+        for (unsigned Part = 0; Part < UF; ++Part) {
+          SmallVector<Value*, 4> Ops;
+          for (unsigned i = 1; i < Gep->getNumOperands(); i++)
+            Ops.push_back(OpsV[i][Part]);
+          Value *GEPBasePtr = OpsV[0][Part];
+          Value *NewGep = Builder.CreateGEP(nullptr, GEPBasePtr, Ops,
+                                            "VectorGep");
+          assert(NewGep->getType()->isVectorTy() && "Expected vector GEP");
+          NewGep = Builder.CreateBitCast(NewGep,
+                                         VectorType::get(Ptr->getType(), VF));
+          VectorGep.push_back(NewGep);
+        }
+      } else
+        VectorGep = getVectorValue(Ptr);
     }
 
----------------
The function is getting too long / indented. I would prefer to see it broken up with helper functions.

================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:2511-2512
@@ +2510,4 @@
+                                            Alignment, MaskPart);
+      }
+      else {
+        // Calculate the pointer for the specific unroll-part.
----------------
Formatting - 'else' should be on the same line as the brace.

================
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.
----------------
typo: transferred

================
Comment at: ../test/Transforms/LoopVectorize/X86/gather_scatter.ll:20-23
@@ +19,6 @@
+;AVX512-LABEL: @foo1
+;AVX512:  llvm.masked.load
+;AVX512: llvm.masked.gather
+;AVX512: llvm.masked.store
+;AVX512: ret void
+
----------------
Please check the full text for any masked ops that are created in this test and others. We do not want to miss any bugs/regressions resulting from changes in the data types or number of instructions produced.

================
Comment at: ../test/Transforms/LoopVectorize/X86/masked_load_store.ll:283
@@ -282,3 +282,3 @@
 ;AVX512-LABEL: @foo4
-;AVX512-NOT: llvm.masked
+;AVX512-NOT: llvm.masked.load
 ;AVX512: ret void
----------------
Please add positive checks for the expected / correct mask ops that will now be produced.


http://reviews.llvm.org/D15690