[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:954
@@ -951,1 +953,3 @@
 
+// If a we have (a) a GEP and (b) a pointer based on an alloca, and the
+// beginning of the object the GEP points would have a negative offset with
----------------
mkuper wrote:
> davidxl wrote:
> > Probably add an example in C form to demonstrate it. Also document key parameters of the method.
> Will do, except that I really think an IR example would be more appropriate here.
You can treat the example as C like pseudo language to make the description more 'high level' and easier to read.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:968
@@ +967,3 @@
+  // If the alloca access size is unknown, or the GEP isn't inbounds, bail.
+  if (GEPMaxLookupReached || AllocaMaxLookupReached ||
+      AllocaAccessSize == MemoryLocation::UnknownSize || !GEPOp->isInBounds())
----------------
mkuper wrote:
> davidxl wrote:
> > Since this method does not need to update the two limits, it is better to move their checks out of line before the call. i.e.:
> > 
> >    if (!GEPMaxLookupReached && !AllocaMaxLookupReached && isGEPBaseAtNegativeOffset(..))
> >        return NoAlias;
> > 
> > This will help make the interface cleaner.
> > 
> > The limit check can also be combined with the next call:
> > 
> >   if (!GEPMaxLookupReached && !AllocaMa... ) {
> >        if (!isGEP....) 
> >           return NoAlias;
> >         if (!isGEP...)
> >            return NoAlias;
> >      }
> > 
> I actually wrote that with the checks out of line first, didn't seem prettier. :-\
> 
> The problem is that the way checkGEP is currently written, there's no clear separation between (1) cases when the second argument is a GEP or not, and (2) cases when the lookup failure should cause it to bail or not. So I have to add extra checks (in the second option you suggest, an extra isGEP check, I can't put the entire isGEP code under a look success check) no matter what,
ok

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1035
@@ +1034,3 @@
+    if (isGEPBaseAtNegativeOffset(GEP2StructOffset, GEP2NonStructOffset,
+                                  GEP1BaseOffset, !GEP2VariableIndices.empty(),
+                                  !GEP1VariableIndices.empty(),
----------------
mkuper wrote:
> davidxl wrote:
> > Perhaps passing the GEPVariableIndices directly to the callee.
> Ditto - I started out with that, and changed it to passing a bool because that's all the helper cares about. Neither version is pretty.
> 
> What do you think about wrapping the entire decomposition result in a struct?
 -- What do you think about wrapping the entire decomposition result in a struct?

I think that will be cleaner -- enhancing the interface in the future does not require updating all use sites -- perhaps extract that as a NFC refactoring patch.


http://reviews.llvm.org/D20495