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

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

Thanks for the comments!



================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1064
+                           SmallVectorImpl<Value *> &Sorted) {
+  SmallVector<std::pair<const SCEV *, Value *>, 4> OffValPairs;
+  Value *Obj = nullptr;
----------------
ABataev wrote:
> You can make `OffValPairs.reserve(VL.size())` and `Sorted.reserve(VL.size())` to avoid possible resize
Right, thanks.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1071
+   
+  // TODO: Do we really need the map, or can we just rely on the SCEV cache?
   for (auto *Val : VL) {
----------------
efriedma wrote:
> The SCEV cache is reasonably fast, but it's obviously faster not to do a hashtable lookup at all.
I know, but using the cache would simplify the code even more, so I wonder if the penalty for a cache lookup is worth it. I mean, a SCEV cache lookup is basically a hash lookup, right?


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1083
+
+    OffValPairs.push_back(std::make_pair(SE.getSCEV(Ptr), Val));
   }
----------------
ABataev wrote:
> Better to use `OffValPairs.emplace_back(SE.getSCEV(Ptr), Val);`
Sure.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1093-1095
+              const SCEVConstant *Diff = dyn_cast<SCEVConstant>(
+                  SE.getMinusSCEV(Right.first, Left.first));
+              return (Diff && (Diff->getAPInt().isStrictlyPositive()));
----------------
mssimpso wrote:
> Do we require constant differences? If not, you could also ask SCEV about the range:
> 
> ```
> return SE.isKnownPositive(SE.getMinusSCEV(Right.first, Left.first))
> ```
@mssimpso : right, I think this should work, thanks! I was stuck thinking about constants because if the differences are non-constant we won't be able to do anything with the sorted array anyway. But this is nicer assuming I can actually use it as a comparator (see answer to Eli).

@efriedma : I know. That's why I've wanted sanjoy to comment on whether we can rely on SCEV to give transitive results. I mean, this comparator is obviously irreflexive, and I'm rather sure it's antisymmetric. So it comes down to whether we can rely on isKnownPositive(getMinus(A,B)) and isKnownPositive(getMinus(B,C)) implying that isKnownPositive(getMinus(A,C)) (or the equivalent for the version I have now).
If not, then I see two options:
(a) roll my own sort that bails if two values are incomparable.
(b) do something like: take one of the pointers, compute the difference vs. all the others, if one of the differences is non-constant bail, otherwise compare based on the diffs.

I guess (b) would be the simplest and safest option.



https://reviews.llvm.org/D29425