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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm created this revision.
arsenm added reviewers: reames, jdoerfert, spatel, efriedma.
Herald added subscribers: hiraditya, wdng.
Herald added a project: LLVM.

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.


https://reviews.llvm.org/D82614

Files:
  llvm/lib/Analysis/ValueTracking.cpp
  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/lib/Analysis/ValueTracking.cpp
===================================================================
--- llvm/lib/Analysis/ValueTracking.cpp
+++ llvm/lib/Analysis/ValueTracking.cpp
@@ -2361,15 +2361,26 @@
     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())
+    if (const Argument *A = dyn_cast<Argument>(V)) {
+      // FIXME: The IR needs a proper constant for invalid pointer values
+      // independent from the "null = 0" constant. Checking for
+      // known-invalid-pointer also needs to be separated from is-known-zero.
+      //
+      // The interpretation of "nonnull" should be a known valid pointer value,
+      // not a 0 bitvalue. As-is, this handling of nonnull is incorrect without
+      // a check for the default address space. However, it seems some users are
+      // relying on this, so leave this as broken for now.
+      if (((A->hasPassPointeeByValueAttr() && PtrTy->getAddressSpace() == 0) ||
+           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.273554.patch
Type: text/x-patch
Size: 2550 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200625/990d4ee6/attachment.bin>