[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 a comment.

Thanks, David!


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:346
@@ -345,3 +345,3 @@
 /// through pointer casts.
-/*static*/ const Value *BasicAAResult::DecomposeGEPExpression(
-    const Value *V, int64_t &BaseOffs,
+const Value *BasicAAResult::DecomposeGEPExpression(
+    const Value *V, int64_t &BaseStructOffs, int64_t &BaseNonStructOffs,
----------------
davidxl wrote:
> Document parameter BaseStructOffs, and BaseNonStructOffs.
Right, thanks.

================
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
----------------
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.

================
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())
----------------
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,

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1035
@@ +1034,3 @@
+    if (isGEPBaseAtNegativeOffset(GEP2StructOffset, GEP2NonStructOffset,
+                                  GEP1BaseOffset, !GEP2VariableIndices.empty(),
+                                  !GEP1VariableIndices.empty(),
----------------
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?


http://reviews.llvm.org/D20495