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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:412
+      break;
+    }
 
----------------
Worth using find_if ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2558
   // Check that none of the instructions in the bundle are already in the tree.
   for (Value *V : VL) {
+    if (!isa<UndefValue>(V) && getTreeEntry(V)) {
----------------
Can we use for (Value *V : make_filter_range(VL, Instruction::classof) ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2570
   // The reduction nodes (stored in UserIgnoreList) also should stay scalar.
   for (Value *V : VL) {
+    if (!isa<UndefValue>(V) &&
----------------
for (Value *V : make_filter_range(VL, Instruction::classof) ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2593
   // Check that every instruction appears once in this bundle.
   SmallVector<unsigned, 4> ReuseShuffleIndicies;
   SmallVector<Value *, 4> UniqueValues;
----------------
Should ReuseShuffleIndicies be SmallVector<int, 4> - and we then tag undefs with -1 (llvm::UndefMaskElem) ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3443
+  const unsigned NumOfInstructions =
+      llvm::count_if(InstructionsOnly, [](Value *V) { return true; });
+  Value *V0;
----------------
can we use llvm::size(InstructionsOnly) ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3871
       }
       for (Value *V : VL) {
+        if (isa<UndefValue>(V))
----------------
InstructionsOnly ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4175
+                           const DenseSet<unsigned> &ShuffledIndices,
+                           const DenseSet<unsigned> &IgnoredIndices) const {
   unsigned NumElts = Ty->getNumElements();
----------------
IgnoredIndices might be cheaper as a SparseBitVector ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4713
+                                       ? cast<Constant>(V)
+                                       : UndefValue::get(LI->getType()));
+        }
----------------
Isn't UndefValue is a type of Constant? Maybe add a comment explaining what you're doing here as its not clear, at least to me.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6156
+    SmallVector<unsigned, 4> NewOrder(Order->begin(), Order->end());
+    SmallVector<bool, 4> UsedIndices(Order->size(), false);
+    unsigned BoundVal = NewOrder.size() + 1;
----------------
Would SmallBitVector be cheaper for UsedIndices ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7281
+        SmallVector<unsigned, 4> NewOrder(Order->begin(), Order->end());
+        SmallVector<bool, 4> UsedIndices(Order->size(), false);
+        unsigned BoundVal = NewOrder.size() + 1;
----------------
SmallBitVector ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57059