[PATCH] D26905: [SLP] Vectorize loads of consecutive memory accesses, accessed in non-consecutive (jumbled) way.

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

Thanks, Shahid.
The rest of my comments are cosmetic - except the one about the sort. I think your sort accidentally ended up quadratic.



================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1034
+    const SCEV *OffsetSCEV = SE.getConstant(Offset);
+    auto *OffsetC = dyn_cast<SCEVConstant>(OffsetSCEV);
+    const APInt &constOffset = OffsetC->getAPInt();
----------------
mkuper wrote:
> If you know this is a SCEVConstant, this should be a cast<>. Otherwise, you need to check the dyn_cast<> actually succeeded.
Shouldn't this sort call be outside the for loop?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:471
+      sortMemAccesses(VL, DL, SE, list);
+      return isSame(list);
+    }
----------------
Can you just use std::equal directly? The only thing isSame() does, aside from that, is assert on the sizes - and you have that assert just a few lines above.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1228
+        sortMemAccesses(VL, *DL, *SE, list);
+        auto newVL = makeArrayRef(list.begin(), list.end());
+        for (unsigned i = 0, e = newVL.size() - 1; i < e; ++i)
----------------
newVL -> NewVL


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1229
+        auto newVL = makeArrayRef(list.begin(), list.end());
+        for (unsigned i = 0, e = newVL.size() - 1; i < e; ++i)
+          if (!isConsecutiveAccess(newVL[i], newVL[i + 1], *DL, *SE)) {
----------------
Could you please add braces to this for? It's a one-statement body, but not a one-line, so I think braces would be better.


https://reviews.llvm.org/D26905