[llvm-commits] [PATCH] Add support to ValueTracking for determining that a pointer is non-null by virtue of inbounds GEPs that preclude a null pointer.

[Chandler Carruth]

  I've addressed all the comments. This patch is passing tests, and Ben already OK'ed it, so I'm gonna go ahead and commit. Let me know if there are more style tweaks you'd like to me to take care of, happy to repaint the bikeshed.

  Also note that Phabricator lost the diffs for the tests I added -- I assure you they're still there.

  Committed in r169573.


================
Comment at: lib/Analysis/ValueTracking.cpp:867-870
@@ +866,6 @@
+///
+/// This routine relies on the GEP being an inbounds GEP in address space zero,
+/// and leverages that to demonstrate that it cannot start or end at a null
+/// pointer while remaining inbounds due to non-zero offsets being applied
+/// during the GEP.
+static bool isGEPKnownNonNull(GEPOperator *GEP, const DataLayout *DL,
----------------
Duncan Sands wrote:
> Bike shed: I think comments explaining how the routine is implemented should be in the body of the routine (eg right after the declaration) rather than before the declaration like here.
This comment dates from when what was discussed here was required for the caller to test. =] I'll update it to reflect the new reality.

================
Comment at: lib/Analysis/ValueTracking.cpp:878
@@ +877,3 @@
+  // inbounds GEP in address space zero.
+  if (isKnownNonZero(GEP->getPointerOperand(), DL, Depth))
+    return true;
----------------
Duncan Sands wrote:
> Here you recurse without checking or updating the depth.  This is not too bad since isKnownNonZero will manage to avoid infinite recursion for you, but it is a bit naughty.
isKnownNonZero increments Depth before calling this routine, so we don't need to increment it here. If you look at the recursive calls inside of isKnownNonZero, none of them increment Depth, relying on the one increment at the top to handle all non-constant cases. This is even true for the two calls over the LHS and RHS of a binary operator.

However, I have a post-increment inside the operand walk so that even if we have 10k operands, we won't recurse 10k times. I only use the depth test when we see non-constant operands though, just is isKnownNonZero doesn't increment for constant operands.

================
Comment at: lib/Analysis/ValueTracking.cpp:881
@@ +880,3 @@
+
+  // Walk the GEP operands and see if there are any non-zero operands. If
+  // so, then the GEP cannot produce a null pointer, as doing so would
----------------
Duncan Sands wrote:
> non-zero operands -> non-zero offsets
> After all, it is perfectly possible to have a non-zero operand that produces a zero
> offset due to zero-size types.
Done.

================
Comment at: lib/Analysis/ValueTracking.cpp:886
@@ +885,3 @@
+       GTI != GTE; ++GTI) {
+    ConstantInt *OpC = dyn_cast<ConstantInt>(GTI.getOperand());
+    if (!OpC && Depth >= MaxDepth)
----------------
Duncan Sands wrote:
> All the mucking around with depths and OpC vs !OpC below is pretty ugly and hard to follow, can you please reorganize it.  For example, here you could immediately do:
>   if (OpC && OpC->isZero())
>     continue;
> rather than bundling it into a GCC-like complicated conditional expression below.
Yea, I found a simpler way to write this. I think this and other things fell out.

================
Comment at: lib/Analysis/ValueTracking.cpp:892-894
@@ +891,5 @@
+    // reason about the GEP.
+    if ((!OpC && !isKnownNonZero(GTI.getOperand(), DL, Depth++)) ||
+        (OpC && OpC->isZero()))
+      continue;
+
----------------
Duncan Sands wrote:
> You could just always test isKnownNonZero rather than specially handling the constant case (as isKnownNonZero knows about constants) and leave mucking about with constants to the logic for struct types.  That would get rid of all the OpC vs !OpC madness.  With the simplest implementation this would mean that you wouldn't catch the constant case if Depth was already maximal, but do we care?
I could, but we need an explicit test on whether the operand is a constant to handle the depth stuff properly, so it seemed like I might as well handle the zero case directly.

Let me know if this is still a problem in your eyes...

================
Comment at: lib/Analysis/ValueTracking.cpp:897
@@ +896,3 @@
+    if (StructType *STy = dyn_cast<StructType>(*GTI)) {
+      assert(OpC && "Only constants can index a struct!");
+      unsigned ElementIdx = OpC->getZExtValue();
----------------
Duncan Sands wrote:
> This might be a vector of constants (vector GEP), though in that case they must all be the same.  You can use Constant::getUniqueInteger to handle this.
We don't get here for a vector GEP. I'll add support for them as a second iteration, already isKnownNonZero is handicapped for vectors. =/

================
Comment at: lib/Analysis/ValueTracking.cpp:899
@@ +898,3 @@
+      unsigned ElementIdx = OpC->getZExtValue();
+      const StructLayout *SL = DL->getStructLayout(STy);
+      uint64_t ElementOffset = SL->getElementOffset(ElementIdx);
----------------
Duncan Sands wrote:
> This will crash if DL is null.
Early exit added.

================
Comment at: lib/Analysis/ValueTracking.cpp:903
@@ +902,3 @@
+        return true;
+    } else if (DL->getTypeAllocSize(GTI.getIndexedType()) > 0) {
+      return true;
----------------
Duncan Sands wrote:
> This will crash if DL is null.
Early exit added.


http://llvm-reviews.chandlerc.com/D160