[PATCH] D64142: [SLP] try to create vector loads from bitcasted scalar pointers

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/SLPVectorizer.h:86
 private:
+  void vectorizeLoads(BasicBlock *BB);
+
----------------
Comment?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5075
   for (auto BB : post_order(&F.getEntryBlock())) {
+    vectorizeLoads(BB);
     collectSeedInstructions(BB);
----------------
Not sure this is the right place to call this function. I would suggest to do it at the end of vectorization. Plus, this should change the value of `Changed` variable.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5250
+    auto *Load = dyn_cast<LoadInst>(&I);
+    if (!Load || Load->isVolatile() || Load->isAtomic())
+      continue;
----------------
`Load->isSimple()` instead of `Load->isVolatile() || Load->isAtomic()`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5268-5272
+    // Do not create a vector load of an unsupported type.
+    auto *VecTy = cast<VectorType>(SrcPtrTy->getElementType());
+    unsigned VecSize = VecTy->getPrimitiveSizeInBits();
+    if (VecSize > TTI->getRegisterBitWidth(true))
+      continue;
----------------
Why? Long vectors could be split into several smaller vector loads successfully.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5280-5306
+    // Original pattern: load (bitcast VecPtr to ScalarPtr)
+    int OldCost = TTI->getMemoryOpCost(Instruction::Load,
+                                       SrcPtrTy->getElementType(), Alignment,
+                                       Load->getPointerAddressSpace());
+    OldCost += TTI->getCastInstrCost(Instruction::BitCast, PtrOpTy, SrcPtrTy);
+
+    // If needed, bitcast the vector type to match the load (scalar element).
----------------
Could you reuse the original logic with building the vectorization tree, cost calculation etc.? This looks like bikeshedding and does not allow to extend it for other ops.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64142/new/

https://reviews.llvm.org/D64142