ConstantFold: Fold out compare of addrspacecasted null with global

[arsenm]

Summary

null in addrspace(0) is defined to not point to any valid object.
Casting the pointer to another address space cannot make it point
to a valid object.

This is consistent with alias analysis's interpretation of null, e.g.

; Function: ptr_compare: 2 pointers, 0 call sites
; MayAlias: i32* %ptr, i32 addrspace(3)* @lds
define void @ptr_compare(i32* %ptr) {

%load = load i32, i32 addrspace(3)* @lds
 store i32 0, i32* %ptr
 ret void

}

; Function: null_compare: 2 pointers, 0 call sites
; NoAlias: i32 addrspace(3)* @lds, i32* null
define void @null_compare() {

%load = load i32, i32 addrspace(3)* @lds
store i32 0, i32* null
ret void

}

Fixes issue 58617, for the basic case. More complex cases,
like an offset from a GEP should also fold out. I'm not sure
it's appropriate for ConstantFolding to handle the general case,
which should use getUnderlyingObject.

[nikic]

I'm not completely convinced that this is correct. Note that icmp on pointers is a pure bitwise comparison, it is not affected by provenance. I think it's pretty clear that ptr null and ptr addrspace(3) @lds are distinct underlying objects, with distinct provenance, and that's all that matters to alias analysis. But it's less obvious to me that casting ptr null to ptr addrspace(3) may not produce a bit pattern that just so happens to match ptr addrspace(3) @lds.

@nlopes This silently fails to compile in the online version: https://alive2.llvm.org/ce/z/XC2RGM Locally it prints:

ERROR: Unsupported instruction:   %__constexpr_0 = icmp eq ptr addrspace(3) addrspacecast (ptr null to ptr addrspace(3)), @lds
ERROR: Unsupported instruction:   ret i1 icmp eq (ptr addrspace(3) addrspacecast (ptr null to ptr addrspace(3)), ptr addrspace(3) @lds)
ERROR: Could not translate 'src' to Alive IR

Would be great if these errors would show up online as well. Maybe the problem is that the error gets printed to stdout rather than stderr?

[nlopes]

In D136882#3891143, @nikic wrote:

I'm not completely convinced that this is correct. Note that icmp on pointers is a pure bitwise comparison, it is not affected by provenance. I think it's pretty clear that ptr null and ptr addrspace(3) @lds are distinct underlying objects, with distinct provenance, and that's all that matters to alias analysis. But it's less obvious to me that casting ptr null to ptr addrspace(3) may not produce a bit pattern that just so happens to match ptr addrspace(3) @lds.

@nlopes This silently fails to compile in the online version: https://alive2.llvm.org/ce/z/XC2RGM Locally it prints:

ERROR: Unsupported instruction:   %__constexpr_0 = icmp eq ptr addrspace(3) addrspacecast (ptr null to ptr addrspace(3)), @lds
ERROR: Unsupported instruction:   ret i1 icmp eq (ptr addrspace(3) addrspacecast (ptr null to ptr addrspace(3)), ptr addrspace(3) @lds)
ERROR: Could not translate 'src' to Alive IR

Would be great if these errors would show up online as well. Maybe the problem is that the error gets printed to stdout rather than stderr?

Indeed, everything gets written to stdout (long story, but it allows us to forward all output to a file; we could have 2 pipes ofc, but..).
@regehr It seems that there might be some regex stuff going on to cleanup the output that is filtering out the errors? Or maybe CE just does that by default when the command fails?

Regarding the optimization itself, doesn't look correct to me either, for the exact reason that Nikita gave.

[nhaehnle]

In D136882#3891143, @nikic wrote:

I'm not completely convinced that this is correct. Note that icmp on pointers is a pure bitwise comparison, it is not affected by provenance. I think it's pretty clear that ptr null and ptr addrspace(3) @lds are distinct underlying objects, with distinct provenance, and that's all that matters to alias analysis. But it's less obvious to me that casting ptr null to ptr addrspace(3) may not produce a bit pattern that just so happens to match ptr addrspace(3) @lds.

In the issue discussion, I hadn't considered the fact that icmp is a pure bitwise comparison. This convinces me that the fold is incorrect without target-specific information.

It is correct for this particular case with target-specific information.

[jdoerfert]

Do we have a target hook isNullPtrValid(unsigned AS)?

[arsenm]

In D136882#3892296, @jdoerfert wrote:

Do we have a target hook isNullPtrValid(unsigned AS)?

No. I would like that information to be encoded in the data layout. I was hoping I could get this fold without doing that much larger change

[nikic]

Marking this as changes requested per above comments.

llvm/lib/IR/ConstantFold.cpp

static ICmpInst::Predicate evaluateICmpRelation(Constant *V1, Constant *V2,
   }
 
   return ICmpInst::BAD_ICMP_PREDICATE;
 }
 
 static Constant *constantFoldCompareGlobalToNull(CmpInst::Predicate Predicate,
                                                  Constant *C1, Constant *C2) {
   const GlobalValue *GV = dyn_cast<GlobalValue>(C2);
-  if (!GV || !C1->isNullValue())
+  if (!GV)
     return nullptr;
 
   // Don't try to evaluate aliases.  External weak GV can be null.
-  if (!isa<GlobalAlias>(GV) && !GV->hasExternalWeakLinkage() &&
+  if (isa<GlobalAlias>(GV) || GV->hasExternalWeakLinkage())
+    return nullptr;
+
+  // Strip away addrspacecast to see if this is derived from null.
+  C1 = C1->stripPointerCasts();
+
+  // A null pointer in the default address space cannot point to any valid
+  // object, regardless of the address space.
+  if (isa<ConstantPointerNull>(C1) &&
       !NullPointerIsDefined(nullptr /* F */,
-                            GV->getType()->getAddressSpace())) {
+                            cast<PointerType>(C1->getType())->getAddressSpace())) {
     if (Predicate == ICmpInst::ICMP_EQ)
       return ConstantInt::getFalse(C1->getContext());
     else if (Predicate == ICmpInst::ICMP_NE)
       return ConstantInt::getTrue(C1->getContext());
   }
 
   return nullptr;
 }

llvm/test/Assembler/constant-fold-compare-null-to-global-issue58617.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -S %s | FileCheck %s
 
 @lds = internal addrspace(3) global [128 x i32] undef, align 16
 
 ; null in addrspace(0) cannot point to a valid object
 define i1 @cmp_addrspacecast_null() {
 ; CHECK-LABEL: @cmp_addrspacecast_null(
 ; CHECK-NEXT:  bb0:
-; CHECK-NEXT:    ret i1 icmp eq (ptr addrspace(3) addrspacecast (ptr null to ptr addrspace(3)), ptr addrspace(3) @lds)
+; CHECK-NEXT:    ret i1 false
 ;
 bb0:
   ret i1 icmp eq (ptr addrspace(3) addrspacecast (ptr null to ptr addrspace(3)), ptr addrspace(3) @lds)
 }
 
 define i1 @cmp_addrspacecast_null_commute() {
 ; CHECK-LABEL: @cmp_addrspacecast_null_commute(
-; CHECK-NEXT:    ret i1 icmp eq (ptr addrspace(3) addrspacecast (ptr null to ptr addrspace(3)), ptr addrspace(3) @lds)
+; CHECK-NEXT:    ret i1 false
 ;
   ret i1 icmp eq (ptr addrspace(3) @lds, ptr addrspace(3) addrspacecast (ptr null to ptr addrspace(3)))
 }
 
 ; FIXME: This should fold out
 define i1 @cmp_addrspacecast_null_global_gep() {
 ; CHECK-LABEL: @cmp_addrspacecast_null_global_gep(
 ; CHECK-NEXT:    ret i1 icmp eq (ptr addrspace(3) addrspacecast (ptr null to ptr addrspace(3)), ptr addrspace(3) getelementptr ([128 x i32], ptr addrspace(3) @lds, i32 16))