[llvm] 0347039 - ValueTracking: Fix isKnownNonZero for non-0 null pointers for byval

[Matt Arsenault via llvm-commits]

Author: Matt Arsenault
Date: 2020-07-16T13:50:49-04:00
New Revision: 0347039a6e7daa774d016e0a4e0f2568c7913351

URL: https://github.com/llvm/llvm-project/commit/0347039a6e7daa774d016e0a4e0f2568c7913351
DIFF: https://github.com/llvm/llvm-project/commit/0347039a6e7daa774d016e0a4e0f2568c7913351.diff

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

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

I think it's not possible to break this for AMDGPU due to the copy
semantics of byval. If you have an original stack object at 0, the
byval copy will be placed above it so I don't think it's really
possible to hit a 0 address.

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

Modified: 
    llvm/lib/Analysis/ValueTracking.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 43caaa62c2ec..306f6a4a35ca 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -2353,15 +2353,20 @@ bool isKnownNonZero(const Value *V, const APInt &DemandedElts, unsigned Depth,
     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))

diff  --git a/llvm/test/Transforms/InstSimplify/null-ptr-is-valid-attribute.ll b/llvm/test/Transforms/InstSimplify/null-ptr-is-valid-attribute.ll
new file mode 100644
index 000000000000..8ce04e7d3a9c
--- /dev/null
+++ b/llvm/test/Transforms/InstSimplify/null-ptr-is-valid-attribute.ll
@@ -0,0 +1,20 @@
+; 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
+}

diff  --git a/llvm/test/Transforms/InstSimplify/null-ptr-is-valid.ll b/llvm/test/Transforms/InstSimplify/null-ptr-is-valid.ll
new file mode 100644
index 000000000000..7b7fb140e8c3
--- /dev/null
+++ b/llvm/test/Transforms/InstSimplify/null-ptr-is-valid.ll
@@ -0,0 +1,24 @@
+; 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
+}