[PATCH] D122145: [SLP] Cluster ordering for loads

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3257
+  unsigned Cnt = 1;
+  for (auto *Ptr : VL.drop_front()) {
+    bool Found = false;
----------------
I beleive better to use `Value *` here instead of `auto *`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3259
+    bool Found = false;
+    for (auto &Base : Bases) {
+      Optional<int> Diff =
----------------
`const auto &`?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3259-3269
+    for (auto &Base : Bases) {
+      Optional<int> Diff =
+          getPointersDiff(ElemTy, Base.first, ElemTy, Ptr, DL, SE,
+                          /*StrictCheck=*/true);
+      if (!Diff)
+        continue;
+
----------------
ABataev wrote:
> `const auto &`?
Better to outline this loop to a lambda.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3266
+
+      Base.second.push_back(std::make_tuple(Ptr, *Diff, Cnt++));
+      Found = true;
----------------
`emplace_back(Ptr, *Diff, Cnt);`. Also, I'm not sure about postincrement here, we usually avoid doing it in the expressions.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3277
+      // Not found already - add a new Base
+      Bases[Ptr].push_back(std::make_tuple(Ptr, 0, Cnt++));
+    }
----------------
`emplace_back(Ptr, 0, Cnt);`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3282
+  bool AnyConsecutive = false;
+  for (auto &Base : Bases) {
+    auto &Vec = Base.second;
----------------
`const auto &`?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3285
+    if (Vec.size() > 1) {
+      llvm::sort(Vec, [](std::tuple<Value *, int, unsigned> &X,
+                         std::tuple<Value *, int, unsigned> &Y) {
----------------
`stable_sort`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3291
+      int End = std::get<1>(Vec.back());
+      AnyConsecutive |= (End - Start) == int(Vec.size() - 1);
+    }
----------------
What if some of the loads are the loads from the same address but different instruction?
Like:
```
%gep0 = gep %0, 1
%l0 = load %gep0
%gep1 = gep %0, 1
%l1 = load %gep1
```
%l0 and %l1 will be in the same vector but they are not consecutive, though `(End - Start) == int(Vec.size() - 1);` might be true for >=4 loads.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3305
+
+  assert(SortedIndices.size() == VL.size());
+  return true;
----------------
Message?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3314
+
+  SmallVector<Value *> Ptrs;
+  for (Value *V : TE.Scalars) {
----------------
Try to preallocate space in the vector, the number of elements is known.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3368-3369
       return CurrentOrder;
+    if (Optional<OrdersType> Order = findPartiallyOrderedLoads(TE))
+      return Order;
   }
----------------
Add a check that we have >= 4 loads


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

https://reviews.llvm.org/D122145