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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3259
+    bool Found = false;
+    for (auto &Base : Bases) {
+      Optional<int> Diff =
----------------
ABataev wrote:
> ABataev wrote:
> > `const auto &`?
> Better to outline this loop to a lambda.
For this one and the other `const auto&` below we might modify Base.


================
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;
+
----------------
dmgreen wrote:
> ABataev wrote:
> > ABataev wrote:
> > > `const auto &`?
> > Better to outline this loop to a lambda.
> For this one and the other `const auto&` below we might modify Base.
Not sure I understood this one exactly, let me know if I got the idea wrong.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3291
+      int End = std::get<1>(Vec.back());
+      AnyConsecutive |= (End - Start) == int(Vec.size() - 1);
+    }
----------------
ABataev wrote:
> 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.
Changing it to checking all the indexes sounds good.


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

https://reviews.llvm.org/D122145