Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename AddrSpacePtr #36059

Merged
merged 2 commits into from
Jun 3, 2020
Merged

Rename AddrSpacePtr #36059

merged 2 commits into from
Jun 3, 2020

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented May 28, 2020

This PR started as a rework of AddrSpacePtr, adding support for it to pointerset and pointerref, but while doing so I realized that (1) it isn't a good fit for Base (e.g. what to do at run-time with non-generic address spaces), and (2) AddrSpacePtr doesn't accurately reflect what this type does: These pointers are lowered to IR in a way that more closely resembles the underlying LLVM pointers:

julia> foo(x) = x
foo (generic function with 1 method)

julia> @code_llvm foo(C_NULL)

;  @ REPL[1]:1 within `foo'
define i64 @julia_foo_726(i64) {
top:
  ret i64 %0
}

julia> @code_llvm foo(reinterpret(Core.LLVMPtr{Nothing,0}, C_NULL))

;  @ REPL[1]:1 within `foo'
define i8* @julia_foo_817(i8*) {
top:
  ret i8* %0
}

i.e. more than just adding address space information, so it seems better to call it LLVMPtr.

I also considered lowering them to non-opaque types, i.e. i64* in the above example, but LLVM itself seems to be moving away from that it seems better not do. (Linked changes don't fully work)

PR also adds an ugly error when using invalid address space typevars -- no way to cleanly emit an error from there.

Open question: add support for pointerset/pointerref with LLVMPtrs, or keep these intrinsics in e.g. LLVM.jl? They currently live in CUDA.jl: https://github.com/JuliaGPU/CUDA.jl/blob/3af3e3104cc7baae2408598220395eff078e3f28/src/device/pointer.jl#L114-L180

Marked for backport so that we don't have the old name in any release.

cc @thomasfaingnaert

@maleadt maleadt added compiler:codegen Generation of LLVM IR and native code gpu Affects running Julia on a GPU backport 1.5 labels May 28, 2020
@maleadt maleadt requested review from vchuravy and Keno May 28, 2020 16:54
@maleadt maleadt merged commit 2855625 into master Jun 3, 2020
@maleadt maleadt deleted the tb/llvmptr branch June 3, 2020 08:38
@vtjnash
Copy link
Member

vtjnash commented Jun 4, 2020

I think, if true, this would indicate it's a flaw in the abstraction level and we should consider removing this and doing it differently, as we generally try to keep LLVM as an implementation detail and avoid using it to define the behavior. However, since address_space is also present in clang and sparse, I think it's inaccurate to claim it's specific to LLVM.

@maleadt
Copy link
Member Author

maleadt commented Jun 5, 2020

we generally try to keep LLVM as an implementation detail and avoid using it to define the behavior

Except for, e.g., llvmcall or the hard-coded use of LLVM intrinsics. It's exactly for those cases that we need to be able to emit something more closely to LLVM's representation (this type doing more than just adding an address-space property).

If Base ever has a need for address spaces, I guess we can always re-add Base.AddrSpacePtr that would still follow Julia's ABI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code gpu Affects running Julia on a GPU
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants