[PATCH] D125111: [SLP] Make reordering aware of external vectorizable scalar stores.

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3547
         if (UserTE->UserTreeIndices.empty())
           UserTE = nullptr;
         else
----------------
not your code but this is unreachable because of check at line 3539.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3589
+      if (!OpTE->ExternalUserReorderIndices.empty()) {
+        for (const OrdersType &Order : OpTE->ExternalUserReorderIndices)
+          ++OrdersUses.insert(std::make_pair(Order, 0)).first->second;
----------------
nit: name Order is already used by lambda above.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4034
+  DenseMap<Value *, SmallVector<StoreInst *, 4>> PtrToStoresMap;
+  for (unsigned Lane = 0, E = NumLanes; Lane != E; ++Lane) {
+    Value *V = TE->Scalars[Lane];
----------------
nit:    for (unsigned Lane : seq<unsigned>(0, TE->Scalars.size())) {
then  NumLanes can be dropped


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4048
+        continue;
+      auto *SI = cast<StoreInst>(U);
+      if (!SI->isSimple())
----------------
nit: use dyn_cast and move it to line 4043 (presumably with isSimple and isValidElementType  checks)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125111