Skip to content

Conversation

gbaraldi
Copy link
Member

Fixes the compile time regression in #59134 (does not address the performance regression that should be tracked and bisected)

@gbaraldi gbaraldi requested a review from vtjnash August 12, 2025 14:17
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be an assert? It appears we only call this when the frontend IR put a check here mandating that the bit pattern be all zeros.

@gbaraldi
Copy link
Member Author

There's a case which I haven't tracked down too well here but that we emit a null check on an alloca. Where we as frontend will comply with AMDGPU and emit the alloca on the correct addresspace

@gbaraldi
Copy link
Member Author

gbaraldi commented Aug 12, 2025

JuliaGPU/AMDGPU.jl#780 i.e here. I think LLVM can and does fold those away for normal pointers but not here

@gbaraldi gbaraldi added the backport 1.12 Change should be backported to release-1.12 label Aug 12, 2025
@gbaraldi
Copy link
Member Author

For code that emits those null checks see

function foo(C, A, ta)
       @inbounds for i in eachindex(C)
         x = ta == 0 ? A[1] : (ta == 1 ? A[1] : A[1])
         C[i] = x
         end
         nothing
         end
@code_llvm optimize=false foo(zeros(ComplexF64,1,1), ones(ComplexF64, 1), 1)

There are at least 3 null checks of allocas here

@vtjnash
Copy link
Member

vtjnash commented Aug 12, 2025

Ah okay right, we use that for emit_isa_and_defined in phi nodes, since it is a simple way to check a lot of possible things, since we cannot know (until runtime) whether that is a pointer to a heap object or alloca or sometimes both or something else. I still think upstream AMD is at fault here, since the C standard explicitly says that that the null pointer definition in any address space can have any bit pattern (e.g. -1) but is guaranteed not to point to a valid object:
https://www.gnu.org/software/libc//manual/html_node/Null-Pointer-Constant.html. It is very awkward then that AMD decided to use the same term to mean the opposite in LLVM: that the null pointer is guaranteed to have bit pattern 0 but might point at a valid object.

@gbaraldi
Copy link
Member Author

The issue is LLVM hard codes null to 0, which is not what C says. And AMD is the only target that actually exposes that. The actual null is never a valid address, which is why in C amdgpu clang will introduce the same thing we're doing here.

@gbaraldi gbaraldi merged commit 920df7a into master Aug 12, 2025
9 checks passed
@gbaraldi gbaraldi deleted the gb/addrspace branch August 12, 2025 17:30
KristofferC pushed a commit that referenced this pull request Aug 13, 2025
Fixes the compile time regression in
#59134 (does not address the
performance regression that should be tracked and bisected)

(cherry picked from commit 920df7a)
@KristofferC KristofferC mentioned this pull request Aug 13, 2025
38 tasks
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Aug 19, 2025
DilumAluthge pushed a commit that referenced this pull request Aug 30, 2025
Fixes the compile time regression in
#59134 (does not address the
performance regression that should be tracked and bisected)

(cherry picked from commit 920df7a)
KristofferC pushed a commit that referenced this pull request Sep 5, 2025
Fixes the compile time regression in
#59134 (does not address the
performance regression that should be tracked and bisected)

(cherry picked from commit 920df7a)
gbaraldi added a commit that referenced this pull request Sep 9, 2025
…ion for null ptr comparisons") to Julia 1.11.x (#59446)

Backports #59259 to 1.11 (cherry picked from commit
920df7a).

For more details, see
#59445 (comment).

Targets `backports-release-1.11` (#59336).

---------

Co-authored-by: Gabriel Baraldi <baraldigabriel@gmail.com>
KristofferC pushed a commit that referenced this pull request Sep 15, 2025
Fixes the compile time regression in
#59134 (does not address the
performance regression that should be tracked and bisected)

(cherry picked from commit 920df7a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants