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

[David Li via llvm-commits]

davidxl added inline comments.

================
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;
----------------
mkuper wrote:
> 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.
You are right that there is no 'inbound' guarantee for sub/inner array we can leverage here (even though accessing a.a[2] will lead to an error when build with -fsanitiize=undefined). Note GCC handles this with -fstrict-aliasing on.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:998
@@ -997,3 @@
-        // assert.
-        if (GEP1BasePtr != UnderlyingV1 || GEP2BasePtr != UnderlyingV2) {
-          return MayAlias;
----------------
mkuper wrote:
> 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.
I mean the part that turns return MayAlias into assert -- not the hoisting part of the change. I think you just need to make that change  (lgtm to that part which can be committed first) and rebase this patch. Or perhaps just leaves the FIXME in this patch (in the hoisted code and clean it up as a follow up.


http://reviews.llvm.org/D20495