[PATCH] D20495: [BasicAA] An inbounds GEP and an alloca can't alias if the base of the GEP would point "below" the alloca

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:355
@@ -356,2 +354,3 @@
+  Decomposed.OtherOffset = 0;
   do {
     // See if this is a bitcast or GEP.
----------------
davidxl wrote:
> Also clear the var indices?
The original code doesn't do this, but I think you're right, it's an oversight. Will add.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:996
@@ +995,3 @@
+  // from the alloca precisely, so no variable indices are allowed.
+  if (!isa<AllocaInst>(DecompAlloca.Base) || !DecompAlloca.VarIndices.empty())
+    return false;
----------------
davidxl wrote:
> Is the VarIndices.empty() check too restrictive? 
> 
> struct A {
>   int a[2];
>   ...
> };
> 
> struct B {
>     int b1;
>     int b2;
>     int b3;
> };
> 
> struct A a;
> B *bp;
> 
> bp->b3 vs a.a[i] should produce no aliasing
I'm not sure this is a good example.

Given:

```
struct __attribute__((packed)) A {
  int a[2]
  int b;
};

struct B {
  int b1;
  int b2;
  int b3;
};

struct A a;
B *bp = (B*)(&a);
```
I'm not sure we want, in -fno-strict-aliasing mode, to conclude that bp->b3 doesn't alias a.a[2] (which since the struct is packed, is a.b) - and so it may also alias a.a[i].

So, it seems to me that this could be less restrictive - but not too much. I think the most we can do if the alloca has variable indices, is take the maximum values that will still leave the whole gep inbounds w.r.t the alloca, and use that. If you think we actually run into those cases in practice, I can try to do that as a followup patch.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:998
@@ -997,3 @@
-        // assert.
-        if (GEP1BasePtr != UnderlyingV1 || GEP2BasePtr != UnderlyingV2) {
-          return MayAlias;
----------------
davidxl wrote:
> Extract this unrelated cleanup into  separate patch.
It's not exactly unrelated.

The old code would call DecomposeGEPExpression on-demand only in code paths that were using the decomposition. Which was, I think, the majority - but not 100%. I needed to hoist the calls up - but I don't think that should go in as a stand-alone patch, since if it goes in without the rest of my changes, it incurs compile-time cost without much benefit.

If you think the clean-up is worth that, I can split it into a separate patch, but I'd rather not.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1035
@@ -1034,3 @@
-    // FIXME: They always have a DataLayout, so this should become an assert.
-    if (GEP1BasePtr != UnderlyingV1 || GEP2BasePtr != UnderlyingV2) {
-      return MayAlias;
----------------
davidxl wrote:
> separate out clean up code into different patch.
Same as above.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1085
@@ -1084,3 @@
-    // FIXME: They always have a DataLayout, so this should become an assert.
-    if (GEP1BasePtr != UnderlyingV1) {
-      return MayAlias;
----------------
davidxl wrote:
> Ditto -- cleanup patch.
Same as above.


http://reviews.llvm.org/D20495