[PATCH] D28044: [LV/LoopAccess] Check statically if an unknown dependence distance can be proven larger than the loop-count

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added inline comments.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1310
+      // (this is equivalent to what the SIV data-dependence test would do).
+      const SCEV *BackedgeTakenCount = PSE.getBackedgeTakenCount();
+      const uint64_t ByteStride = Stride * TypeByteSize;
----------------
dorit wrote:
> dorit wrote:
> > mkuper wrote:
> > > Can you use getMaxBackedgeTakenCount()?
> > > If the count is precisely known, then they should be equivalent. If it's not, then since by definition MaxBackedgeTakenCount >= BackedgeTakenCount, the equation above also holds.
> > > 
> > > (I'm not sure if ever vectorize when we don't have a precise getBackedgeTakenCount(), but even if we don't, no need for extra constraints here)
> > Ok; I gave this a try, but getMaxBackedgeTakenCount is giving me -1… maybe I'm using it wrong?:
> > Const SCEV *MaxBackedgeTakenCount = PSE.getSE()->getMaxBackedgeTakenCount(InnermostLoop);
> > 
> > I don't see any uses of this API in the vectorizer/LoopAccesses; is it supposed to work also when predicates are added to compute the BackedgeCount?
> > 
> > (BTW, we indeed never get here if getBackedgeTakenCount is UnknownSCEV. it' a requirement in canAnalyzeLoop).
> >  getMaxBackedgeTakenCount is giving me -1
> 
> probably this related to the PR you opened (PR31412)? (although this is not in the remainder loop...?)
Err, yes, you're right, this is currently broken. Sorry for the confusion.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1325
+
+      // Is (Dist > takenCount * stepInBytes) ?
+      // (If so, then we have proven (**) because |dist| >= dist)
----------------
dorit wrote:
> mkuper wrote:
> > (Sorry for possibly rehashing the discussion you had with Silviu)
> > The code looks fine, but I'm not entirely sure I understand why this is the best way to approach this. I've read through PR31098, and it probably contains the information I'm looking for, but it's spread over separate messages and it's a bit hard for me to follow.
> > 
> > 1) Why can't you offload this into SCEV? Is the problem representing |dist| as a SCEV, or that SCEV can't prove |dist| - product is positive?
> > 2) While this may be a good idea independently, wouldn't cases like PR31098, in general be better served by improving alias analysis, and then being able to prove that 8D >= 0?
> 1. AFAICS, SCEV can't answer the questions isKnownPositive(dist)/isKnownNonNegative(dist)/… for the expression in question. It looks like SCEV can tell that D = (%size /u 2) is positive,  but it can't tell that 8*D is positive... (where in fact it is even strictly positive if we know we entered the loop). So I think there's room for improvement in that respect.           But in any case, even if/when that is improved, there still may be scenarios in which we can't answer the isKnownPositive question about dist, but we can successfully prove that either (dist - product > 0) or (-dist - product > 0) simply because things get canceled out...  (After all, the goal is not to compute |dist|, but to prove the inequality...).
> 
> 2. By improving alias analysis you refer to my suggestion to be able to anti-alias structure fields? If so then yes, sure, that would help prune irrelevant dependencies. (But if we had a regular array strided access like so: a[2*i+1], a[2*i+D] then alias analysis would not come to the rescue… we'd have to deal with dist =  8*D-4 for which 8*D>=0 is not sufficient)
> 
1. What I mean is that this holds unconditionally:

```
(X - Y > 0 || -X - Y > 0) <=> |X| - Y > 0

```
Would it make sense to sink this inference into SCEV itself, instead of only using it here? If not, is it because the SCEV representation of |X| is too hairy?

2. Yes, that's what I meant. And you're right, we probably want both.


https://reviews.llvm.org/D28044