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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev 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())) {
----------------
spatel wrote:
> 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?
I was just trying to protect the code and try to support it only for simple types at first. There are some doubts that the cost for masked loads/stores is completed and I protected it to make it work only for simple types. I can remove this check if the cost model for masked ops is good enough.


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4743
+        SmallVector<Constant *, 4> Mask;
+        SmallVector<Constant *, 4> Passthrough;
+        Mask.reserve(E->Scalars.size());
----------------
spatel wrote:
> 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.
Fixed


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4815-4816
+      Value *VecPtr;
+      if (!IsPowOf2NumOfInstructions ||
+          NumOfInstructions == E->Scalars.size() || NumOfInstructions == 1) {
+        VecPtr = Builder.CreateBitCast(ScalarPtr,
----------------
spatel wrote:
> 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.
Fixed, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57059