Add amdgpu_dispatch_ptr intrinsic #150607
Conversation
rustbot
added
A-LLVM
|
I can help with design review but not for the implementation, sorry. Regarding the design, if the return type is "erased" I would suggest using a raw pointer instead of a reference. |
|
yoink. If anyone wants to chip in on the review, please feel free, I just want to make sure I have a gander before it ships so I can keep vaguely abreast of what's happening in this space. |
library/core/src/intrinsics/mod.rs
Outdated
| #[rustc_intrinsic] | ||
| #[cfg(target_arch = "amdgpu")] | ||
| #[must_use = "returns a reference that does nothing unless used"] | ||
| pub fn amdgpu_dispatch_ptr() -> &'static (); |
There was a problem hiding this comment.
Maybe we should have a new "amdgpu" file or so for this, to keep it separate from the typically more portable intrinsics in the rest of this file?
Or a new "gpu" file that offload also goes into? I don't know what a sensible grouping here would look like.
There was a problem hiding this comment.
gpu.rs seems like a good place to start, to me, that way other gpu targets are encouraged to reuse code from amdgpu by generalizing it instead of repeating it (as we have discussed before, GPU targets are like siblings: they make much of their tiny differences, while being mostly similar).
There was a problem hiding this comment.
Glancing at NVPTX, it seems they achieve the same things as AMDGPU here by having special registers that are read, whereas AMDGPU uses the struct pointer, so at least for this case they will differ.
There was a problem hiding this comment.
( given their code is JITted by the device driver anyways, for all I know they both actually use the same pattern in the actual machine code they lower to. )
Flakebi
force-pushed
the
dispatch-ptr-intrinsic
branch
from
1d98b13 to
13d7a3c
Compare
|
Thanks for the quick reviews! cc @ZuseZ4 FYI for the |
There was a problem hiding this comment.
This seems to be conformant to the idea that @nikic recommended, of keeping the address space out of the public API, and the code is otherwise fine, so it should be good.
I recommend commenting on the load-bearing implication, though, before we send this in.
workingjubilee
added
S-waiting-on-author
Flakebi
force-pushed
the
dispatch-ptr-intrinsic
branch
from
13d7a3c to
fe4a9b4
Compare
|
I added the suggested comment. (For context, as far as I know, pointercast – in the age of opaque pointers – does only one of these two things:
FWIW, I understood nikic’s comment on the addrspace PR as that we probably don’t need to expose address spaces publicly for address spaces convertible to the generic address space. |
|
Indeed, I'm just noting it makes it very hidden behind the API in this case by not even letting the addrspace leave the compiler! If we want to maybe put a type in @bors r+ rollup |
bors
added
S-waiting-on-bors
…ubilee Add amdgpu_dispatch_ptr intrinsic There is an ongoing discussion in rust-lang#150452 about using address spaces from the Rust language in some way. As that discussion will likely not conclude soon, this PR adds one rustc_intrinsic with an addrspacecast to unblock getting basic information like launch and workgroup size and make it possible to implement something like `core::gpu`. Add a rustc intrinsic `amdgpu_dispatch_ptr` to access the kernel dispatch packet on amdgpu. The HSA kernel dispatch packet contains important information like the launch size and workgroup size. The Rust intrinsic lowers to the `llvm.amdgcn.dispatch.ptr` LLVM intrinsic, which returns a `ptr addrspace(4)`, plus an addrspacecast to `addrspace(0)`, so it can be returned as a Rust reference. The returned pointer/reference is valid for the whole program lifetime, and is therefore `'static`. The return type of the intrinsic (`&'static ()`) does not mention the struct so that rustc does not need to know the exact struct type. An alternative would be to define the struct as lang item or add a generic argument to the function. Is this ok or is there a better way (also, should it return a pointer instead of a reference)? Short version: ```rust #[cfg(target_arch = "amdgpu")] pub fn amdgpu_dispatch_ptr() -> *const (); ``` Tracking issue: rust-lang#135024 r? RalfJung as you are already aware of the background (feel free to re-assign)
…ubilee Add amdgpu_dispatch_ptr intrinsic There is an ongoing discussion in rust-lang#150452 about using address spaces from the Rust language in some way. As that discussion will likely not conclude soon, this PR adds one rustc_intrinsic with an addrspacecast to unblock getting basic information like launch and workgroup size and make it possible to implement something like `core::gpu`. Add a rustc intrinsic `amdgpu_dispatch_ptr` to access the kernel dispatch packet on amdgpu. The HSA kernel dispatch packet contains important information like the launch size and workgroup size. The Rust intrinsic lowers to the `llvm.amdgcn.dispatch.ptr` LLVM intrinsic, which returns a `ptr addrspace(4)`, plus an addrspacecast to `addrspace(0)`, so it can be returned as a Rust reference. The returned pointer/reference is valid for the whole program lifetime, and is therefore `'static`. The return type of the intrinsic (`&'static ()`) does not mention the struct so that rustc does not need to know the exact struct type. An alternative would be to define the struct as lang item or add a generic argument to the function. Is this ok or is there a better way (also, should it return a pointer instead of a reference)? Short version: ```rust #[cfg(target_arch = "amdgpu")] pub fn amdgpu_dispatch_ptr() -> *const (); ``` Tracking issue: rust-lang#135024 r? RalfJung as you are already aware of the background (feel free to re-assign)
…ubilee Add amdgpu_dispatch_ptr intrinsic There is an ongoing discussion in rust-lang#150452 about using address spaces from the Rust language in some way. As that discussion will likely not conclude soon, this PR adds one rustc_intrinsic with an addrspacecast to unblock getting basic information like launch and workgroup size and make it possible to implement something like `core::gpu`. Add a rustc intrinsic `amdgpu_dispatch_ptr` to access the kernel dispatch packet on amdgpu. The HSA kernel dispatch packet contains important information like the launch size and workgroup size. The Rust intrinsic lowers to the `llvm.amdgcn.dispatch.ptr` LLVM intrinsic, which returns a `ptr addrspace(4)`, plus an addrspacecast to `addrspace(0)`, so it can be returned as a Rust reference. The returned pointer/reference is valid for the whole program lifetime, and is therefore `'static`. The return type of the intrinsic (`&'static ()`) does not mention the struct so that rustc does not need to know the exact struct type. An alternative would be to define the struct as lang item or add a generic argument to the function. Is this ok or is there a better way (also, should it return a pointer instead of a reference)? Short version: ```rust #[cfg(target_arch = "amdgpu")] pub fn amdgpu_dispatch_ptr() -> *const (); ``` Tracking issue: rust-lang#135024 r? RalfJung as you are already aware of the background (feel free to re-assign)
Rollup of 11 pull requests Successful merges: - #149976 (Add waker_fn and local_waker_fn to std::task) - #150074 (Update provider API docs) - #150094 (`c_variadic`: provide our own `va_arg` implementation for more targets) - #150164 (rustc: Fix `-Zexport-executable-symbols` on wasm) - #150569 (Ensure that static initializers are acyclic for NVPTX) - #150607 (Add amdgpu_dispatch_ptr intrinsic) - #150694 (./x check miri: enable check_only feature) - #150717 (Thread `--jobs` from `bootstrap` -> `compiletest` -> `run-make-support`) - #150736 (Add AtomicPtr::null) - #150787 (Add myself as co-maintainer for s390x-unknown-linux-musl) - #150789 (Fix copy-n-paste error in `vtable_for` docs) r? @ghost
rust-bors
bot
added
S-waiting-on-author
|
Commit fe4a9b4 has been unapproved. |
Add a rustc intrinsic `amdgpu_dispatch_ptr` to access the kernel dispatch packet on amdgpu. The HSA kernel dispatch packet contains important information like the launch size and workgroup size. The Rust intrinsic lowers to the `llvm.amdgcn.dispatch.ptr` LLVM intrinsic, which returns a `ptr addrspace(4)`, plus an addrspacecast to `addrspace(0)`, so it can be returned as a Rust reference. The returned pointer/reference is valid for the whole program lifetime, and is therefore `'static`. The return type of the intrinsic (`*const ()`) does not mention the struct so that rustc does not need to know the exact struct type. An alternative would be to define the struct as lang item or add a generic argument to the function. Short version: ```rust #[cfg(target_arch = "amdgpu")] pub fn amdgpu_dispatch_ptr() -> *const (); ```
Flakebi
force-pushed
the
dispatch-ptr-intrinsic
branch
from
fe4a9b4 to
91d4e40
Compare
|
I fixed it to pass with |
7 participants
There is an ongoing discussion in #150452 about using address spaces from the Rust language in some way.
As that discussion will likely not conclude soon, this PR adds one rustc_intrinsic with an addrspacecast to unblock getting basic information like launch and workgroup size and make it possible to implement something like
core::gpu.Add a rustc intrinsic
amdgpu_dispatch_ptrto access the kernel dispatch packet on amdgpu.The HSA kernel dispatch packet contains important information like the launch size and workgroup size.
The Rust intrinsic lowers to the
llvm.amdgcn.dispatch.ptrLLVM intrinsic, which returns aptr addrspace(4), plus an addrspacecast toaddrspace(0), so it can be returned as a Rust reference.The returned pointer/reference is valid for the whole program lifetime, and is therefore
'static.The return type of the intrinsic (
&'static ()) does not mention the struct so that rustc does not need to know the exact struct type. An alternative would be to define the struct as lang item or add a generic argument to the function.Is this ok or is there a better way (also, should it return a pointer instead of a reference)?
Short version:
Tracking issue: #135024
r? RalfJung as you are already aware of the background (feel free to re-assign)