This is an archive of the discontinued LLVM Phabricator instance.

ConstantFold: Fold out compare of addrspacecasted null with global
Needs RevisionPublic

Authored by arsenm on Oct 27 2022, 1:40 PM.

Details

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.

Diff Detail

Event Timeline

arsenm created this revision.Oct 27 2022, 1:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 1:40 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
arsenm requested review of this revision.Oct 27 2022, 1:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 1:40 PM
Herald added a subscriber: wdng. · View Herald Transcript
nikic added a subscriber: nlopes.Oct 28 2022, 1:41 AM

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 added a subscriber: regehr.Oct 28 2022, 2:19 AM

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.

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.

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

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 requested changes to this revision.Oct 30 2022, 1:09 AM

Marking this as changes requested per above comments.

This revision now requires changes to proceed.Oct 30 2022, 1:09 AM