[PATCH] D66664: [FIX] Nonnull is not always implied by dereferenceable

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert created this revision.
jdoerfert added reviewers: reames, sanjoy, hfinkel, chandlerc, efriedma, lebedev.ri, fhahn.
Herald added subscribers: bollu, hiraditya, tpr.
Herald added a project: LLVM.

Depending on the function and address space nunnull is not implied by
dereferenceable. With this patch Value::getPointerDereferenceableBytes
will take into account that NULL might be a dereferenceable pointer.

This shows the problem and a solution but the TODO mentions how we
should actually go about it. See also D66618 <https://reviews.llvm.org/D66618>

This will also affect the following not yet patched tests:
 CodeGen/AMDGPU/legalize-fp-load-invariant.ll
 CodeGen/AMDGPU/parallelandifcollapse.ll
 Transforms/InstCombine/addrspacecast.ll
 Transforms/InstCombine/memcpy-addrspace.ll


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66664

Files:
  llvm/lib/IR/Value.cpp
  llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll


Index: llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll
===================================================================
--- llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll
+++ llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll
@@ -51,10 +51,16 @@
     %sret_gep_outside = getelementptr %struct.A, %struct.A* %result, i64 0, i32 1, i64 7
     load i8, i8* %sret_gep_outside
 
-; CHECK: %dparam{{.*}}(aligned)
+; FIXME: This can be null but it is also known dereferenceable. However, right
+;        now we cannot return both information from
+;        Value::getPointerDereferenceableBytes(...).
+; CHECK-NOT: %dparam{{.*}}(aligned)
     %load3 = load i32, i32 addrspace(1)* %dparam
 
-; CHECK: %relocate{{.*}}(aligned)
+; FIXME: This can be null but it is also known dereferenceable. However, right
+;        now we cannot return both information from
+;        Value::getPointerDereferenceableBytes(...).
+; CHECK-NOT: %relocate{{.*}}(aligned)
     %tok = tail call token (i64, i32, i1 ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_i1f(i64 0, i32 0, i1 ()* @return_i1, i32 0, i32 0, i32 0, i32 0, i32 addrspace(1)* %dparam)
     %relocate = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token %tok, i32 7, i32 7)
     %load4 = load i32, i32 addrspace(1)* %relocate
@@ -106,8 +112,11 @@
     %load14 = load i8, i8* @globalptr.align16, align 16
 
     ; Loads from aligned arguments
-; CHECK: %dparam.align1{{.*}}(unaligned)
-; CHECK: %dparam.align16{{.*}}(aligned)
+; FIXME: This can be null but it is also known dereferenceable. However, right
+;        now we cannot return both information from
+;        Value::getPointerDereferenceableBytes(...).
+; CHECK-NOT: %dparam.align1{{.*}}(unaligned)
+; CHECK-NOT: %dparam.align16{{.*}}(aligned)
     %load15 = load i8, i8 addrspace(1)* %dparam.align1, align 16
     %load16 = load i8, i8 addrspace(1)* %dparam.align16, align 16
 
@@ -132,10 +141,13 @@
     %load18 = load i1, i1* %alloca.align16, align 16
 
     ; Loads from GEPs
-; CHECK: %gep.align1.offset1{{.*}}(unaligned)
-; CHECK: %gep.align16.offset1{{.*}}(unaligned)
-; CHECK: %gep.align1.offset16{{.*}}(unaligned)
-; CHECK: %gep.align16.offset16{{.*}}(aligned)
+; FIXME: This can be null but it is also known dereferenceable. However, right
+;        now we cannot return both information from
+;        Value::getPointerDereferenceableBytes(...).
+; CHECK-NOT: %gep.align1.offset1{{.*}}(unaligned)
+; CHECK-NOT: %gep.align16.offset1{{.*}}(unaligned)
+; CHECK-NOT: %gep.align1.offset16{{.*}}(unaligned)
+; CHECK-NOT: %gep.align16.offset16{{.*}}(aligned)
     %gep.align1.offset1 = getelementptr inbounds i8, i8 addrspace(1)* %dparam.align1, i32 1
     %gep.align16.offset1 = getelementptr inbounds i8, i8 addrspace(1)* %dparam.align16, i32 1
     %gep.align1.offset16 = getelementptr inbounds i8, i8 addrspace(1)* %dparam.align1, i32 16
Index: llvm/lib/IR/Value.cpp
===================================================================
--- llvm/lib/IR/Value.cpp
+++ llvm/lib/IR/Value.cpp
@@ -613,9 +613,14 @@
                                                bool &CanBeNull) const {
   assert(getType()->isPointerTy() && "must be pointer");
 
+  const Function *F = nullptr;
+  if (auto *I = dyn_cast<Instruction>(this))
+    F = I->getFunction();
+
   uint64_t DerefBytes = 0;
   CanBeNull = false;
   if (const Argument *A = dyn_cast<Argument>(this)) {
+    F = A->getParent();
     DerefBytes = A->getDereferenceableBytes();
     if (DerefBytes == 0 && (A->hasByValAttr() || A->hasStructRetAttr())) {
       Type *PT = cast<PointerType>(A->getType())->getElementType();
@@ -672,6 +677,12 @@
       CanBeNull = false;
     }
   }
+
+  // Even if we know it is "dereferenceable", it can still be null if null is a
+  // valid pointer. This is a problem as "can be null" is overloaded to mean,
+  // "equals NULL" and "or is not dereferenceable".
+  // TODO: Add a separate flag to communicate "IsKnownDeref".
+  CanBeNull |= NullPointerIsDefined(F, getType()->getPointerAddressSpace());
   return DerefBytes;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66664.216880.patch
Type: text/x-patch
Size: 4078 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190823/5adc43d8/attachment.bin>