-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
Offload safe mutable args with Region and PartitioningStrategy
#158076
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1469,6 +1469,7 @@ symbols! { | |
| off, | ||
| offload, | ||
| offload_kernel, | ||
| offload_region, | ||
| offset, | ||
| offset_of, | ||
| offset_of_enum, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,3 +101,95 @@ macro_rules! offload { | |
| (@value (SOME $val:expr)) => { $val }; | ||
| (@value ($val:expr)) => { $val }; | ||
| } | ||
|
|
||
| // Region & Partitioning Strategy | ||
|
|
||
| /// Defines how execution units access memory regions. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// Implementations must guarantee that generated views are disjoint. | ||
| #[unstable(feature = "offload", issue = "124509")] | ||
| pub unsafe trait PartitioningStrategy { | ||
| /// Read-only view type for the partitioned memory region. | ||
| type View<'a, T: 'a>; | ||
|
|
||
| /// Mutable view type for the partitioned memory region. | ||
| type ViewMut<'a, T: 'a>; | ||
|
|
||
| /// Returns the execution index of the current unit. | ||
| fn index() -> usize; | ||
|
|
||
| /// Returns a read-only view of the region for the current execution context. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// `ptr` must point to `len` valid, initialized elements of type `T`. | ||
| /// The memory must stay valid for lifetime `'a`. | ||
| unsafe fn get<'a, T>(ptr: *const T, len: usize) -> Option<Self::View<'a, T>>; | ||
|
|
||
| /// Returns a mutable view of the region for the current execution context. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// `ptr` must point to `len` valid, initialized elements of type `T`. | ||
| /// The memory must stay valid for lifetime `'a`. | ||
| /// The returned view must be disjoint from all other active views. | ||
| unsafe fn get_mut<'a, T>(ptr: *mut T, len: usize) -> Option<Self::ViewMut<'a, T>>; | ||
| } | ||
|
|
||
| /// A memory region bound to a partitioning strategy. | ||
| #[derive(Copy, Clone, Debug)] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think copy/clone are sound here. After all we intentionally made the members private to prevent this: #[offload_kernel]
fn k(mut a: Region<f32, Linear1D>) {
let mut b = a; // Copy
if let (Some(x), Some(y)) = (a.get_mut(), b.get_mut()) { *x = 1.0; *y = 2.0; } // two live &mut f32 to the same element
}We also don't really need or use them that way anywhere. Can you drop them and add a test to make sure it doesn't compile? |
||
| #[unstable(feature = "offload", issue = "124509")] | ||
| #[rustc_diagnostic_item = "offload_region"] | ||
| pub struct Region<'a, T, S: PartitioningStrategy> { | ||
| ptr: *mut T, | ||
| len: usize, | ||
| _marker: core::marker::PhantomData<(&'a mut [T], S)>, | ||
| } | ||
|
|
||
| /// Raw representation used to build a [`Region`] from common aggregate types. | ||
| #[derive(Debug)] | ||
| #[unstable(feature = "offload", issue = "124509")] | ||
| pub struct RawRegion<'a, T> { | ||
| ptr: *mut T, | ||
| len: usize, | ||
| _marker: core::marker::PhantomData<&'a mut [T]>, | ||
| } | ||
|
|
||
| impl<'a, T> From<&'a mut [T]> for RawRegion<'a, T> { | ||
| fn from(data: &'a mut [T]) -> Self { | ||
| Self { ptr: data.as_mut_ptr(), len: data.len(), _marker: core::marker::PhantomData } | ||
| } | ||
| } | ||
|
|
||
| impl<'a, T, const N: usize> From<&'a mut [T; N]> for RawRegion<'a, T> { | ||
| fn from(data: &'a mut [T; N]) -> Self { | ||
| Self { ptr: data.as_mut_ptr(), len: N, _marker: core::marker::PhantomData } | ||
| } | ||
| } | ||
|
|
||
| #[unstable(feature = "offload", issue = "124509")] | ||
| impl<'a, T, S: PartitioningStrategy> Region<'a, T, S> { | ||
| /// Creates a new partitioned region from data convertible into a [`RawRegion`]. | ||
| pub fn new<D>(data: D) -> Self | ||
| where | ||
| D: Into<RawRegion<'a, T>>, | ||
| { | ||
| let raw = data.into(); | ||
| Self { ptr: raw.ptr, len: raw.len, _marker: core::marker::PhantomData } | ||
| } | ||
|
|
||
| /// Returns a read-only view for the current execution context. | ||
| pub fn get(&self) -> Option<S::View<'_, T>> { | ||
| // SAFETY: `self.ptr` points to `self.len` valid elements for lifetime `'a`. | ||
| unsafe { S::get(self.ptr as *const T, self.len) } | ||
| } | ||
|
|
||
| /// Returns a mutable view for the current execution context. | ||
| pub fn get_mut(&mut self) -> Option<S::ViewMut<'_, T>> { | ||
| // SAFETY: `self.ptr` points to `self.len` valid elements for lifetime `'a`. | ||
| // The strategy guarantees that the returned view is disjoint. | ||
| unsafe { S::get_mut(self.ptr, self.len) } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| //@ compile-flags: -Zoffload=Test -Zunstable-options -C opt-level=1 -Clto=fat | ||
| //@ no-prefer-dynamic | ||
| //@ needs-offload | ||
|
|
||
| // This test verifies that a `Region` kernel argument is mapped like a slice. | ||
| #![feature(abi_gpu_kernel)] | ||
| #![feature(core_intrinsics)] | ||
| #![feature(gpu_offload)] | ||
| #![feature(offload)] | ||
| #![feature(rustc_attrs)] | ||
| #![no_main] | ||
|
|
||
| extern crate core; | ||
|
|
||
| use core::offload::{PartitioningStrategy, Region}; | ||
|
|
||
| struct Dummy; | ||
|
|
||
| unsafe impl PartitioningStrategy for Dummy { | ||
| type View<'a, T: 'a> = &'a T; | ||
| type ViewMut<'a, T: 'a> = &'a mut T; | ||
|
|
||
| fn index() -> usize { | ||
| 0 | ||
| } | ||
|
|
||
| unsafe fn get<'a, T>(_ptr: *const T, _len: usize) -> Option<Self::View<'a, T>> { | ||
| None | ||
| } | ||
|
|
||
| unsafe fn get_mut<'a, T>(_ptr: *mut T, _len: usize) -> Option<Self::ViewMut<'a, T>> { | ||
| None | ||
| } | ||
| } | ||
|
|
||
| // CHECK: @anon.[[ID:.*]].0 = private unnamed_addr constant [23 x i8] c";unknown;unknown;0;0;;\00", align 1 | ||
|
|
||
| // CHECK-DAG: @.offload_sizes.[[K:[^ ]*foo]] = private unnamed_addr constant [2 x i64] [i64 0, i64 8] | ||
| // CHECK-DAG: @.offload_maptypes.[[K]].begin = private unnamed_addr constant [2 x i64] [i64 1, i64 768] | ||
| // CHECK-DAG: @.offload_maptypes.[[K]].kernel = private unnamed_addr constant [2 x i64] [i64 32, i64 800] | ||
| // CHECK-DAG: @.offload_maptypes.[[K]].end = private unnamed_addr constant [2 x i64] [i64 2, i64 0] | ||
|
|
||
| // CHECK: define{{( dso_local)?}} void @main() | ||
| // CHECK: %.offload_sizes = alloca [2 x i64], align 8 | ||
| // CHECK: call void @llvm.memcpy.p0.p0.i64(ptr {{.*}} %.offload_sizes, ptr {{.*}} @.offload_sizes.[[K]], i64 16, i1 false) | ||
| // CHECK: store i64 16, ptr %.offload_sizes, align 8 | ||
| // CHECK: call void @__tgt_target_data_begin_mapper(ptr nonnull @anon.[[ID]].1, i64 -1, i32 2, ptr nonnull %.offload_baseptrs, ptr nonnull %.offload_ptrs, ptr nonnull %.offload_sizes, ptr nonnull @.offload_maptypes.[[K]].begin, ptr null, ptr null) | ||
| // CHECK: call i32 @__tgt_target_kernel(ptr nonnull @anon.[[ID]].1, i64 -1, i32 1, i32 1, ptr nonnull @.[[K]].region_id, ptr nonnull %kernel_args) | ||
| // CHECK-NEXT: call void @__tgt_target_data_end_mapper(ptr nonnull @anon.[[ID]].1, i64 -1, i32 2, ptr nonnull %.offload_baseptrs, ptr nonnull %.offload_ptrs, ptr nonnull %.offload_sizes, ptr nonnull @.offload_maptypes.[[K]].end, ptr null, ptr null) | ||
|
|
||
| #[unsafe(no_mangle)] | ||
| fn main() { | ||
| let mut x = [0.0f32; 4]; | ||
| core::intrinsics::offload::<_, _, ()>( | ||
| foo, | ||
| [1, 1, 1], | ||
| [1, 1, 1], | ||
| 0, | ||
| (Region::<f32, Dummy>::new(&mut x as &mut [f32]),), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a test for when we try to pass &Region? I think I did that accidentally when writing some of our benchmarks and iirc it didn't get caught anywhere but resulted in a buggy runtime.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, can you add a test to show that the borrowchecker rejects writing into x here while the Region is alive? Similar to the preload test here (which I need to clean up too) main...ZuseZ4:rust:offload-explicit-datatransfer3#diff-f828186e6b7ed6e21e5a04161c08c8aa3d03f107a62d728e516c5741b6d6a21a |
||
| ); | ||
| } | ||
|
|
||
| fn foo(region: Region<'_, f32, Dummy>) { | ||
| unreachable!(); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait a second :D #124509
View changes since the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol