[PATCH] D101555: [SLP]Improve handling of compensate external uses cost.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

A few minors



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2913
+      constexpr int NumOps = 2;
+      SmallVector<ValueList, NumOps> VectorOperands(NumOps);
+      for (int I = 0; I < NumOps; ++I) {
----------------
SmallVector seems unnecessary - why not just ValueList VectorOperands[NumOps] ? Even NumOps seems a bit too much.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3821
+        Value *V = VL[I];
+        Optional<int> InsertIdx = getInsertIndex(V, 0);
+        if (!InsertIdx || *InsertIdx == UndefMaskElem)
----------------
V is used only once
```
getInsertIndex(VL[I], 0)
```


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3833
+        ShuffleMask[Idx] = I;
+      }
 
----------------
assert(Offset < UINT_MAX && "Failed to find vector index offset") ? Or should it be Offset < NumScalars ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4381
+    Instruction *IE1 = cast<InsertElementInst>(V1);
+    Instruction *IE2 = cast<InsertElementInst>(V2);
+
----------------
auto


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4429
+          }
+        }
+        if (VecId < 0) {
----------------
Can this be replaced with a if (none_of(FirstUsers)) pattern? You might be able to merge AreFromSingleVector into the lambda as well, although that might get too unwieldy?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4986
+        }
+      }
+
----------------
assert(Offset < UINT_MAX && "Failed to find vector index offset") ? Or should it be Offset < NumScalars ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101555