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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
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()));
----------------
mkuper wrote:
> mkuper wrote:
> > efriedma wrote:
> > > mkuper wrote:
> > > > 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.
> > > > 
> > > I don't think we can rely on SCEV being transitive?  At least, I wouldn't want to rely on it, then crash in some obscure case.
> > > 
> > > Anyway, your comparator has worse problems: currently, you can prove 0 == A == 1 with it.
> > > I don't think we can rely on SCEV being transitive? At least, I wouldn't want to rely on it, then crash in some obscure case.
> > That worries me too, that's why I wanted a stamp of approval/disapproval from sanjoy :-)
> > Maybe I'll just go with (b) and avoid this can of worms.
> > 
> > > Anyway, your comparator has worse problems: currently, you can prove 0 == A == 1 with it.
> > I don't see why. I mean, for {0, 1, A} the strict weak order is "0 < 1, A is incomparable with both 0 and 1". The only comparison that evaluates to true is cmp(0,1), and this should be fine.
> Er, wait, you're right, that's not strict. But I think it still satisfies the explicit requirements for a comparator.
C++ [alg.sorting]p3; "For algorithms [...] to work correctly, comp has to induce a strict weak ordering on the values."


https://reviews.llvm.org/D29425