Skip to content

Offload safe mutable args with Region and PartitioningStrategy - #158076

Open
Sa4dUs wants to merge 3 commits into
rust-lang:mainfrom
Sa4dUs:offload-region
Open

Offload safe mutable args with Region and PartitioningStrategy#158076
Sa4dUs wants to merge 3 commits into
rust-lang:mainfrom
Sa4dUs:offload-region

Conversation

@Sa4dUs

@Sa4dUs Sa4dUs commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Now Region are lang items and mapped as slices.
needs #156620 to work

r? @ZuseZ4

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 18, 2026
@rust-log-analyzer

This comment has been minimized.

@Sa4dUs
Sa4dUs force-pushed the offload-region branch 2 times, most recently from c075a56 to 2a627bf Compare August 13, 2026 14:05
@rust-log-analyzer

This comment has been minimized.

@Sa4dUs Sa4dUs changed the title offload region Offload safe mutable args with Region and PartitioningStrategy Aug 13, 2026
@Sa4dUs
Sa4dUs marked this pull request as ready for review August 13, 2026 16:21
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 13, 2026
@rust-bors

This comment has been minimized.

@rustbot

This comment has been minimized.

Comment thread library/core/src/offload/mod.rs Outdated
}

/// A memory region bound to a partitioning strategy.
#[derive(Copy, Clone, Debug)]

@ZuseZ4 ZuseZ4 Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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?
For convenience you can instead probably add something like
fn reborrow(&mut self) -> Region<'_, T, S> to reuse it accross launches

View changes since the review

Comment thread tests/codegen-llvm/gpu_offload/region_host.rs Outdated
Comment thread library/core/src/offload/mod.rs Outdated
/// # Safety
///
/// Implementations must guarantee that generated views are disjoint.
#[unstable(feature = "offload", issue = "124509")]

@ZuseZ4 ZuseZ4 Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

lol

@rustbot

rustbot commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@ZuseZ4

ZuseZ4 commented Sep 5, 2026

Copy link
Copy Markdown
Member

Next round. I think we should actually refuse all cases (for now) where we have a Region within any ADT/struct/slice, etc. not just &Region. Most of them would be transfered incorrectly, and I don't think we gain much. The other direction is something like Region<Vec<f32>, S>, where we also transfer T incorrectly. But to be fair, I think that direction is already unchecked on main. I assume that offloading a vec will still do the wrong thing, so I'm fine if we accept that here as well till we implement a proper handling. But also feel free to already reject it already if you want.

where
Ty<'tcx>: TyAbiInterface<'tcx, C>,
{
if let Some(elem_ty) = region_element_ty(tcx, ty) {

@ZuseZ4 ZuseZ4 Sep 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is only correct by default, not under -Zrandomize-layout, so you should either mark Region as repr(C), or properly look up the order. I'd to the later, just out of principle so we give fewer guarantees to the user.

View changes since the review

#[unstable(feature = "offload", issue = "131513")]
#[rustc_diagnostic_item = "offload_region"]
pub struct Region<'a, T, S: PartitioningStrategy> {
ptr: *mut T,

@ZuseZ4 ZuseZ4 Sep 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should be safe to promise NonNull<T> here and below.

View changes since the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants