[PATCH] D12064: [GMR] isNonEscapingGlobalNoAlias() should look through Bitcasts/GEPs when looking at loads.

[hfinkel@anl.gov via llvm-commits]

hfinkel added inline comments.

================
Comment at: lib/Analysis/IPA/GlobalsModRef.cpp:654
@@ -653,3 +653,3 @@
       // that the global is non-escaping, so no alias.
-      if (isa<GlobalValue>(LI->getPointerOperand()))
+      if (isa<GlobalValue>(GetUnderlyingObject(LI->getPointerOperand(), *DL)))
         continue;
----------------
chandlerc wrote:
> hfinkel wrote:
> > It seems like we could make this more general is two ways:
> > 
> >  1. Call GetUnderlyingObjects, and checking that all of the results are GlobalValues
> > 
> >  2. Also checking that the underlying objects are not arguments or call/invoke return values.
> > 
> > If I'm reading this code correctly, one possible way of handling this, is just to do this:
> > 
> >   push.Inputs(LI->getPointerOperand());
> >   continue;
> > 
> > which seems to accomplish both things, and keeps an integrated Depth constraint.
> > 
> You're saying a pointer loaded from an escaping pointer is itself escaping?
> 
> I think I agree, but it makes this surprisingly more powerful. This will chase a chain of loads until it hits control flow or a known underlying object. Nifty, but maybe surprising.
> 
> However, unless I'm mistaken, we still need to call GetUnderlyingObject? The worklist doesn't do that.
> 
> I'm a bit scared about using GetUnderlyingObjects making this significantly more slow...
Responses below, but actually, it seems that we can do even better than this:

A non-addr-taken global does not have its address stored anywhere, ever. Thus, if we get a pointer from a load, it cannot be a non-addr-taken global (except for those in AllocsForIndirectGlobals). No?

> You're saying a pointer loaded from an escaping pointer is itself escaping?

Yes, I think that's correct.

> I think I agree, but it makes this surprisingly more powerful. This will chase a chain of loads until it hits control flow or a known underlying object. Nifty, but maybe surprising.

With a depth limit of 4, it is not going to do very *much* chasing ;) - but, yes, that's the idea.

> However, unless I'm mistaken, we still need to call GetUnderlyingObject? The worklist doesn't do that.

Ah, yes, correct. The existing worklist does not look though GEPs. Should it?

> I'm a bit scared about using GetUnderlyingObjects making this significantly more slow...

Indeed; this was my motivation for suggesting using the existing worklist (to get the unified depth limit).




http://reviews.llvm.org/D12064