ValueTracking: Fix isKnownNonZero for non-0 null pointers for byval

[arsenm]

Summary

The IR doesn't have a proper concept of invalid pointers, and "null"
constants are just all zeros (though it really needs one).

The nonnull attribute blurs this distinction, and handling in it
isKnownNonZero without checking for the default address space is also
simply wrong. However, it seems some statepoint/gc.relocate users are
relying on the current behavior in addrspace(1). This will probably
need fixing as well, since I believe I recently saw a case where this
was suspect.

[efriedma]

llvm/lib/Analysis/ValueTracking.cpp

2378

LangRef says nonnnull means "not the value null", and the value null is an all-zero bit pattern. That's maybe not ideal for some targets, sure, but I don't think there's a reason to call it out here specifically. If we decide to change the meaning of the attributes in question, we'll audit all the users.

2379

Please use NullPointerIsDefined.

[jdoerfert]

Honestly, nonnull should only mean != 0. I guess not deducing nonnull from byval is reasonable for any AS != 0.
I'm not really in favor of making nonnull mean "valid pointer", dereferenceable is already way closer to that and nonnull is used as "not 0" too often already.

[arsenm]

In D82614#2115882, @jdoerfert wrote:

Honestly, nonnull should only mean != 0. I guess not deducing nonnull from byval is reasonable for any AS != 0.
I'm not really in favor of making nonnull mean "valid pointer", dereferenceable is already way closer to that and nonnull is used as "not 0" too often already.

Without a marker for a known-invalid pointer, I don't see how you can repurpose dereferenceable to optimize out "null" checks for other invalid pointer values. The useful property is this pointer is known to never be a valid object; we don't have an isKnownNotDereferenceable

[jdoerfert]

In D82614#2115906, @arsenm wrote:

In D82614#2115882, @jdoerfert wrote:

Honestly, nonnull should only mean != 0. I guess not deducing nonnull from byval is reasonable for any AS != 0.
I'm not really in favor of making nonnull mean "valid pointer", dereferenceable is already way closer to that and nonnull is used as "not 0" too often already.

Without a marker for a known-invalid pointer, I don't see how you can repurpose dereferenceable to optimize out "null" checks for other invalid pointer values. The useful property is this pointer is known to never be a valid object; we don't have an isKnownNotDereferenceable

Fair, but reusing nonnull is not the solution. A "null" check, i.a. ptr == 0, is where nonnull comes in. For the lack of an alternative people started to overload the meaning. This is problematic but I guess already happening. I mean, nonnull cannot be equivalent to != 0 and at the same time equivalent to "not a valid pointer" because that would mean != 0 is equivalent to "not a valid pointer" (which it is not). I think both interpretations have been used in the past. I argue we need to fix it to one and introduce something else for the other. I would strongly favor != 0 as the canonical meaning of nonnull, basically how I read the langref right now. Unclear if "not a valid pointer" needs to be an attribute but maybe.

[arsenm]

Use NullPointerIsDefined. I realized it's not actually possible to break this for AMDGPU due to the implicit copy, but I think this is still probably wrong by the IR rules as written. I guess your stack address space could wrap around?

[jdoerfert]

I think this is conceptually the right thing to do. Short from us finally adding more target knowledge hooks for AS questions we should include this.

[arsenm]

0347039a6e7daa774d016e0a4e0f2568c7913351

llvm/lib/Analysis/ValueTracking.cpp

bool isKnownNonZero(const Value *V, const APInt &DemandedElts, unsigned Depth,
   if (isKnownNonZeroFromAssume(V, Q))
     return true;
 
   // Some of the tests below are recursive, so bail out if we hit the limit.
   if (Depth++ >= MaxDepth)
     return false;
 
   // Check for pointer simplifications.
-  if (V->getType()->isPointerTy()) {
+
+  if (PointerType *PtrTy = dyn_cast<PointerType>(V->getType())) {
     // Alloca never returns null, malloc might.
     if (isa<AllocaInst>(V) && Q.DL.getAllocaAddrSpace() == 0)
       return true;
 
-    // A byval, inalloca, or nonnull argument is never null.
-    if (const Argument *A = dyn_cast<Argument>(V))
-      if (A->hasPassPointeeByValueAttr() || A->hasNonNullAttr())
+    // A byval, inalloca may not be null in a non-default addres space. A
+    // nonnull argument is assumed never 0.
+    if (const Argument *A = dyn_cast<Argument>(V)) {
+      if (((A->hasPassPointeeByValueAttr() &&
+            !NullPointerIsDefined(A->getParent(), PtrTy->getAddressSpace())) ||
+           A->hasNonNullAttr()))
         return true;
+    }
 
     // A Load tagged with nonnull metadata is never null.
     if (const LoadInst *LI = dyn_cast<LoadInst>(V))
       if (Q.IIQ.getMetadata(LI, LLVMContext::MD_nonnull))
         return true;
 
     if (const auto *Call = dyn_cast<CallBase>(V)) {
       if (Call->isReturnNonNull())
         return true;
       if (const auto *RP = getArgumentAliasingToReturnedPointer(Call, true))

llvm/test/Transforms/InstSimplify/null-ptr-is-valid-attribute.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -instsimplify < %s | FileCheck %s
+
+; A 0 valued byval pointer may be valid
+define i1 @byval_may_be_zero(i32* byval(i32) %ptr) null_pointer_is_valid {
+; CHECK-LABEL: @byval_may_be_zero(
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32* [[PTR:%.*]], null
+; CHECK-NEXT:    ret i1 [[CMP]]
+;
+  %cmp = icmp eq i32* %ptr, null
+  ret i1 %cmp
+}
+
+define i1 @nonnull_may_be_zero(i32* nonnull %ptr) null_pointer_is_valid {
+; CHECK-LABEL: @nonnull_may_be_zero(
+; CHECK-NEXT:    ret i1 false
+;
+  %cmp = icmp eq i32* %ptr, null
+  ret i1 %cmp
+}

llvm/test/Transforms/InstSimplify/null-ptr-is-valid.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -instsimplify < %s | FileCheck %s
+
+target datalayout = "A5"
+
+; A 0 valued byval pointer may be valid
+define i1 @byval_may_be_zero(i32 addrspace(5)* byval(i32) %ptr) {
+; CHECK-LABEL: @byval_may_be_zero(
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 addrspace(5)* [[PTR:%.*]], null
+; CHECK-NEXT:    ret i1 [[CMP]]
+;
+  %cmp = icmp eq i32 addrspace(5)* %ptr, null
+  ret i1 %cmp
+}
+
+; FIXME: The interpretation of nonnull assumes a 0 pointer value, so
+; this really is an incorrect fold.
+define i1 @nonnull_may_be_zero(i32 addrspace(5)* nonnull %ptr) {
+; CHECK-LABEL: @nonnull_may_be_zero(
+; CHECK-NEXT:    ret i1 false
+;
+  %cmp = icmp eq i32 addrspace(5)* %ptr, null
+  ret i1 %cmp
+}