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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1452
+        (-GEP1BaseOffset).uge(V2Size.getValue()))
+      return NoAlias;
 
----------------
asbirlea wrote:
> 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.
I've now added a test that asserted with the previous code. The thing to keep in mind here is that while the GEP offset is negative, the access size still points in the positive direction. So for the negative case, what we need is that access 1 fits between the negative GEP1BaseOffset and zero, because zero is where the access 2 starts.


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

https://reviews.llvm.org/D91383