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

Allow MaybeUninit in input and output of inline assembly #114790

Merged
merged 1 commit into from Aug 23, 2023

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Aug 14, 2023

Motivation:

As part of the work to remove UBs from crossbeam's AtomicCell, I'm writing a library to implement atomic operations on MaybeUnint using inline assembly (atomic-maybe-uninit, crossbeam-rs/crossbeam#1015).

However, currently, MaybeUnint cannot be used in input&output of inline assembly, so when processing MaybeUninit, values must be passed through memory. It is inefficient and microbenchmarks have actually shown significant performance degradation.

It would be nice if we could allow MaybeUninit in input and output of inline assembly.


This PR changed the type check in rustc_hir_analysis to allow MaybeUnint<int | float | ptr | fn ptr | simd vector> in input and output of inline assembly and added a simple test.

To be honest, I'm not sure that this is the correct way to do it, because this is like doing transmute to integers/floats/etc from MaybeUninit on the compiler side. EDIT: this seems fine

r? @Amanieu
cc @thomcc (because you had previously proposed this)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 14, 2023
@RalfJung
Copy link
Member

The real property we care about here is that this is a repr(transparent) type. However we currently have no way to indicate whether repr(transparent) is a public guarantee vs a private implementation detail, so it should probably not be used in type-checking.

@taiki-e
Copy link
Member Author

taiki-e commented Aug 14, 2023

The real property we care about here is that this is a repr(transparent) type. However we currently have no way to indicate whether repr(transparent) is a public guarantee vs a private implementation detail, so it should probably not be used in type-checking.

Hmm, I think this PR is fine on this because it targets only MaybeUninit, not arbitrary union or repr(transparent) type, and it is guaranteed that the layout of MaybeUninit<T> is the same as T.

Comment on lines 152 to 144
ty::Adt(adt, args) if Some(adt.did()) == self.tcx.lang_items().maybe_uninit() => {
let fields = &adt.non_enum_variant().fields;
let ty = fields[FieldIdx::from_u32(1)].ty(self.tcx, args);
let ty::Adt(ty, args) = ty.kind() else { unreachable!() };
assert!(ty.is_manually_drop());
let fields = &ty.non_enum_variant().fields;
let ty = fields[FieldIdx::from_u32(0)].ty(self.tcx, args);
self.get_asm_ty(ty, expr, is_input)?
}
Copy link
Member Author

@taiki-e taiki-e Aug 23, 2023

Choose a reason for hiding this comment

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

We can support MaybeUninit<MaybeUninit<...>> (nested MaybeUninit) by moving this arm to the get_asm_ty function, should we support that?

(I don't know of any specific use cases where supporting it would be useful.)

@Amanieu
Copy link
Member

Amanieu commented Aug 23, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 23, 2023

📌 Commit 03fd2d4 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2023
@bors
Copy link
Contributor

bors commented Aug 23, 2023

⌛ Testing commit 03fd2d4 with merge 97fff1f...

@bors
Copy link
Contributor

bors commented Aug 23, 2023

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 97fff1f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 23, 2023
@bors bors merged commit 97fff1f into rust-lang:master Aug 23, 2023
12 checks passed
@taiki-e taiki-e deleted the asm-maybe-uninit branch August 23, 2023 15:34
@rustbot rustbot added this to the 1.74.0 milestone Aug 23, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (97fff1f): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.6% [-2.0%, -1.1%] 6
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.7% [-5.1%, -1.2%] 14
Improvements ✅
(secondary)
-2.4% [-2.4%, -2.4%] 1
All ❌✅ (primary) -2.7% [-5.1%, -1.2%] 14

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 635.819s -> 634.534s (-0.20%)
Artifact size: 347.15 MiB -> 347.13 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants