[FIX] Nonnull is not always implied by dereferenceable

[jdoerfert]

Summary

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

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

[efriedma]

getPointerDereferenceableBytes returns some number of dereferenceable bytes. If CanBeNull is true, that result is modified: if the pointer value is null, the number of known dereferenceable bytes is actually zero.

As far as I can tell, without your patch, the function implements this logic correctly, and the callers use the result correctly. "CanBeNull == false" doesn't mean the pointer is non-null; it means the caller doesn't have to prove the value is non-null before using the "known dereferenceable bytes" result. The parameter CanBeNull should probably be named to clarify that, though; maybe "OrNull" would be better?

[jdoerfert]

In D66664#1643543, @efriedma wrote:

getPointerDereferenceableBytes returns some number of dereferenceable bytes. If CanBeNull is true, that result is modified: if the pointer value is null, the number of known dereferenceable bytes is actually zero.

That is the part that is not true because we conflate two concepts, "can be a nullptr" and "is known to be deref". If the pointer value is null the there can well be dereferenceable bytes. And if there are dereferenceable bytes, the value can as well be null.

As far as I can tell, without your patch, the function implements this logic correctly, and the callers use the result correctly. "CanBeNull == false" doesn't mean the pointer is non-null; it means the caller doesn't have to prove the value is non-null before using the "known dereferenceable bytes" result. The parameter CanBeNull should probably be named to clarify that, though; maybe "OrNull" would be better?

I don't believe it makes sense to cling on the "null is special" concept (e.g., by naming it OrNull) here given that we give up on that for non-0 address spaces.
In D66618 I made the different cases explicit.

[efriedma]

If you want to really expand out the meaning of "CanBeNull", it means "was the number of dereferenceable bytes computed using a dereferenceable_or_null attribute/metadata". It has nothing to do with whether a null pointer is generally valid in the given address space. The logic has always worked this way, since before it was extracted into a separate function in D17572.

getPointerDereferenceableBytes is not a good API, sure; if you want to refactor it, fine. But this patch just breaks it.

[joerg]

It would be nice to also default to this for -ffreestanding.

[jdoerfert]

In D66664#1643716, @efriedma wrote:

If you want to really expand out the meaning of "CanBeNull", it means "was the number of dereferenceable bytes computed using a dereferenceable_or_null attribute/metadata". It has nothing to do with whether a null pointer is generally valid in the given address space. The logic has always worked this way, since before it was extracted into a separate function in D17572.

The logic always worked this way is hardly an argument, given that dereferencebale alone is already misused (=broken). While initially constructed for something, "CanBeNull" seems like a freestanding definition of the return value to me.

getPointerDereferenceableBytes is not a good API, sure; if you want to refactor it, fine.

That is what I proposed to do in my RFC to the list and prototyped already (D66618). Please take a look.

But this patch just breaks it.

I doubt it breaks the API, arguably it makes CanBeNull return false only if the pointer "cannot be null", which seems logical to me. When we additionally return "IsKnownDeref" (see D66618) we do not regress anything (probably what you mean by break here) and we could start to use the logic in the isKnownNonZero instead of duplicating stuff.

In D66664#1644161, @joerg wrote:

It would be nice to also default to this for -ffreestanding.

What default do you want exactly? Did you see D66618 and the RFC email I send to the list?

[jdoerfert]

(This will never go in like this, I will keep it open nor for the discussions sake, D66618 is a better solution)

[lebedev.ri]

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.

llvm/lib/IR/Value.cpp

 const Value *Value::stripInBoundsOffsets() const {
   return stripPointerCastsAndOffsets<PSK_InBounds>(this);
 }
 
 uint64_t Value::getPointerDereferenceableBytes(const DataLayout &DL,
                                                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();
       if (PT->isSized())
         DerefBytes = DL.getTypeStoreSize(PT);
     }
     if (DerefBytes == 0) {
       DerefBytes = A->getDereferenceableOrNullBytes();
uint64_t Value::getPointerDereferenceableBytes(const DataLayout &DL,
   } else if (auto *GV = dyn_cast<GlobalVariable>(this)) {
     if (GV->getValueType()->isSized() && !GV->hasExternalWeakLinkage()) {
       // TODO: Don't outright reject hasExternalWeakLinkage but set the
       // CanBeNull flag.
       DerefBytes = DL.getTypeStoreSize(GV->getValueType());
       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;
 }
 
 unsigned Value::getPointerAlignment(const DataLayout &DL) const {
   assert(getType()->isPointerTy() && "must be pointer");
 
   unsigned Align = 0;
   if (auto *GO = dyn_cast<GlobalObject>(this)) {

llvm/test/Analysis/ValueTracking/memory-dereferenceable.ll

 ; CHECK: %sret_gep{{.*}}(aligned)
     %sret_gep = getelementptr inbounds %struct.A, %struct.A* %result, i64 0, i32 1, i64 2
     load i8, i8* %sret_gep
 
 ; CHECK-NOT: %sret_gep_outside
     %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
 
 ; CHECK-NOT: %nparam
     %dpa = call i32 addrspace(1)* @func1(i32 addrspace(1)* %dparam)
     %nparam = getelementptr i32, i32 addrspace(1)* %dpa, i32 5
     %load5 = load i32, i32 addrspace(1)* %nparam
; CHECK-NOT: %outside_allocation
 
     ; Loads from aligned globals
 ; CHECK: @globalptr.align1{{.*}}(unaligned)
 ; CHECK: @globalptr.align16{{.*}}(aligned)
     %load13 = load i8, i8* @globalptr.align1, align 16
     %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
 
     ; Loads from byval arguments
 ; CHECK: %i8_byval{{.*}}(aligned)
     %i8_byval_load = load i8, i8* %i8_byval
 
 ; CHECK-NOT: %byval_cast
     %byval_cast = bitcast i8* %i8_byval to i32*
     %bad_byval_load = load i32, i32* %byval_cast
 
 ; CHECK: %byval_gep{{.*}}(aligned)
     %byval_gep = getelementptr inbounds %struct.A, %struct.A* %A_byval, i64 0, i32 1, i64 2
     load i8, i8* %byval_gep
 
     ; Loads from aligned allocas
 ; CHECK: %alloca.align1{{.*}}(unaligned)
 ; CHECK: %alloca.align16{{.*}}(aligned)
     %alloca.align1 = alloca i1, align 1
     %alloca.align16 = alloca i1, align 16
     %load17 = load i1, i1* %alloca.align1, align 16
     %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
     %gep.align16.offset16 = getelementptr inbounds i8, i8 addrspace(1)* %dparam.align16, i32 16
     %load19 = load i8, i8 addrspace(1)* %gep.align1.offset1, align 16
     %load20 = load i8, i8 addrspace(1)* %gep.align16.offset1, align 16
     %load21 = load i8, i8 addrspace(1)* %gep.align1.offset16, align 16
     %load22 = load i8, i8 addrspace(1)* %gep.align16.offset16, align 16