[PATCH] D91383: [BasicAA] Make alias GEP positive offset handling symmetric

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1452
+        (-GEP1BaseOffset).uge(V2Size.getValue()))
+      return NoAlias;
 
----------------
nikic wrote:
> asbirlea wrote:
> > jdoerfert wrote:
> > > nikic wrote:
> > > > jdoerfert wrote:
> > > > > Copy paste error?
> > > > Um, where? I'm not seeing it ^
> > > I think I was simply confused. Maybe today is not the day to finish this review (for me). I'll give it another shot tomorrow ;)
> > I believe this is correct :-).
> I think @jdoerfert was right, and there is a mistake here: This should be comparing against `V1Size` rather than `V2Size`.
> 
> ```
> // Non-negative case:
> 0 ... V2Size ... GEP1BaseOffset ... GEP1BaseOffset+V1Size
> // Non-positive case:
> GEP1BaseOffset ... GEP1BaseOffset+V1Size ... 0 ... V2Size
> ```
> We need to check that V1Size fits between GEP1BaseOffset and 0, the V2Size access is above zero and not relevant.
> 
> I'll have to add a test where both access sizes are different.
Adding a test will be the best solution here, but here's where I went back and forth here and I ended up concluding the above is correct.
```
// Non-negative case:
0 ... V2 ... GEP1BasePoint
//       \____/<- V2 needs to fit here;
// All offsets are +,  and GEP1BaseOffset already had GEP2 subtracted.

// Non-positive case:
// Initializing like above: "AllNonPositive = GEP1BaseOffset.isNonPositive();", means:
GEP1BasePoint ... V2 ... 0
// All offsets are -, so it's still V2Size that needs to fit between V2 and GEP1BasePoint.
```

Initially I thought we need to fit V1Size, and in order to match the condition for non-negatives (and fit V2) you'd need to initialize `AllNonPositive = GEP1BaseOffset.isNonNegative();`
So you'd get:
`V2 ... GEP1BasePoint ... 0`
But the indices are negative as well.

Again, +1 on a test to clarify either way.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91383/new/

https://reviews.llvm.org/D91383