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

[Duncan Sands]

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

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

================
Comment at: lib/Analysis/ValueTracking.cpp:878
@@ +877,3 @@
+  // inbounds GEP in address space zero.
+  if (isKnownNonZero(GEP->getPointerOperand(), DL, Depth))
+    return true;
----------------
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.

================
Comment at: lib/Analysis/ValueTracking.cpp:886
@@ +885,3 @@
+       GTI != GTE; ++GTI) {
+    ConstantInt *OpC = dyn_cast<ConstantInt>(GTI.getOperand());
+    if (!OpC && Depth >= MaxDepth)
----------------
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.

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

================
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();
----------------
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.

================
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);
----------------
This will crash if DL is null.


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

BRANCH
  master

ARCANIST PROJECT
  llvm