[PATCH] D57059: [SLP] Initial support for the vectorization of the non-power-of-2 vectors.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3172-3174
+          !(VL0->getOperand(0)->getType()->isIntegerTy() ||
+            VL0->getOperand(0)->getType()->isFloatingPointTy() ||
+            VL0->getOperand(0)->getType()->isPointerTy())) {
----------------
Use isValidElementType() or check for undef directly?
I still can't tell from the debug statement exactly what we are guarding against. Should the type check already be here even without this patch?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3839
+      bool IsPowOf2NumOfInstructions = llvm::isPowerOf2_32(InstrDist);
+      if (!IsPowOf2NumOfInstructions || InstrDist == 1) {
+        IntrinsicCostAttributes Attrs(
----------------
Are we always creating a masked load for a vector with 2 elements? This logic needs a code comment to explain the cases.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4717
+      Value *VecPtr;
+      if (!IsPowOf2NumOfInstructions ||
+          NumOfInstructions == E->Scalars.size() || NumOfInstructions == 1) {
----------------
Please add code comment/example to explain what the difference is between these 2 clauses.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4743
+        SmallVector<Constant *, 4> Mask;
+        SmallVector<Constant *, 4> Passthrough;
+        Mask.reserve(E->Scalars.size());
----------------
Is Passthrough a full vector of undef elements? If so, it should be created/named that way (or directly in the call to CreateMaskedLoad()) rather than in the loop.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4815-4816
+      Value *VecPtr;
+      if (!IsPowOf2NumOfInstructions ||
+          NumOfInstructions == E->Scalars.size() || NumOfInstructions == 1) {
+        VecPtr = Builder.CreateBitCast(ScalarPtr,
----------------
Similar to above (so can we add a helper function to avoid duplicating the code?):
Please add code comment/example to explain what the difference is between these 2 clauses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57059