[PATCH] D82614: ValueTracking: Fix isKnownNonZero for non-0 null pointers for byval

[Matt Arsenault via Phabricator via llvm-commits]

arsenm updated this revision to Diff 274124.
arsenm added a comment.

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82614/new/

https://reviews.llvm.org/D82614

Files:
  llvm/lib/Analysis/ValueTracking.cpp
  llvm/test/Transforms/InstSimplify/null-ptr-is-valid-attribute.ll
  llvm/test/Transforms/InstSimplify/null-ptr-is-valid.ll


Index: llvm/test/Transforms/InstSimplify/null-ptr-is-valid.ll
===================================================================
--- /dev/null
+++ 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
+}
Index: llvm/test/Transforms/InstSimplify/null-ptr-is-valid-attribute.ll
===================================================================
--- /dev/null
+++ 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
+}
Index: llvm/lib/Analysis/ValueTracking.cpp
===================================================================
--- llvm/lib/Analysis/ValueTracking.cpp
+++ llvm/lib/Analysis/ValueTracking.cpp
@@ -2361,15 +2361,20 @@
     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))


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D82614.274124.patch
Type: text/x-patch
Size: 3055 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200629/779a78dc/attachment-0001.bin>