[PATCH] D29425: [SLP] Use SCEV to sort memory accesses

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

Going back a couple of diffs, I don't think SCEV can guarantee beyond "best effort" that `f(A, B) = isKnownPositive(getMinus(A, B))` (or the variant of this that you had) is transitive.  This is because there are places where SCEV gives up on canonicalization after a certain cutoff point (e.g. around large `(x + y) * (a + b + c)` like cases) and so it is possible that while both `B - A` and `C - B` simplify to a constant, `C - A` does not (PR25170 is a more extreme version of this issue, for instance).

However, what you have now looks fine to me.



================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:696
+/// If all pointers refer to the same object, and the differences between all
+/// pointer operands are known to be constant, the array is sorted by offset,
+/// and returned in \p Sorted.
----------------
I don't know general LAA conventions, but this contract seem weird -- why not return an error.

[edit: I noticed you've already answered this]


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1096
+
+    OffValPairs.emplace_back(
+        std::make_pair(Diff->getAPInt().getSExtValue(), Val));
----------------
Do you need the `std::make_pair`?  Can't you do `emplace_back(Diff->getAPInt().getSExtValue(), Val)`?


https://reviews.llvm.org/D29425