Use copy bound in atomic operations to generate simpler MIR

[tmiasko]

No description provided.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[sfackler]

r? @rust-lang/compiler

[eddyb]

@tmiasko Can you add them to the atomic intrinsics as well? Assuming it works on them.

Either way, this PR seems correct in that using the atomic intrinsics with non-Copy types is effectively like using ptr::read to "move out" a value, which is likely UB for any shared memory.

cc @RalfJung @Amanieu

[tmiasko]

I was looking at this from the angle: what would it take for MIR transforms to optimize a.store(v, Ordering::Relaxed); to just an intrinsic call. This change helps, but inlining cost is still above the threshold for store operation in particular (there are costly panic calls, and even replacing them with an abort intrinsic it is merely sufficient to remove one layer of indirection and inline atomic_store function into the method).

Extending this to intrinsics should work. I could add them with understanding that it would break generic code that uses them without similar bounds, like for example this one here (before changes). In a separate PR preferably.

[oli-obk]

what would it take for MIR transforms to optimize a.store(v, Ordering::Relaxed); to just an intrinsic call.

We could extend the #[inline] syntax to have conditions like

#[inline(always, if_arg_is_const(order))]

To get a poor man's version of partial specialization (after which you most often can properly inline).

In general I'm fine with any kind of intrinsic wrapper just becoming #[inline(always)]

[Centril]

Either way, this PR seems correct in that using the atomic intrinsics with non-Copy types is effectively like using ptr::read to "move out" a value, which is likely UB for any shared memory.

N.B: It's only UB if the function is actually reached at runtime.

[RalfJung]

ptr::read behavior does not depend on whether T is Copy or not. So I don't see any relation to UB here.

But until we have a good example for a non-Copy type one might want to use here, there's still no reason not to do this I think. :D

[eddyb]

N.B: It's only UB if the function is actually reached at runtime.

Well, yeah, I meant that using them to duplicate/"move out" a value is likely UB, not just having around the function definitions.

So I don't see any relation to UB here.

What I mean is an atomic load of e.g. a Box<T> value is likely prone to UB if the underlying memory is actually shared cross-thread, as it duplicates a non-Copy value.
ptr::read on non-Copy types requires some form of ownership to prevent later UB, which is mostly antithetical to situations atomics are used in.

[eddyb]

We could extend the #[inline] syntax to have conditions like

#[inline(always, if_arg_is_const(order))]

Heh, this is the sort of thing I was telling @wesleywiser we should compute instead of "inline costs" - conditions under which the function would become trivial.

[tmiasko]

The changes here don't affect any public API (the other PR is more interesting in that respect).

r? @eddyb

[eddyb]

@bors r+

[bors]

📌 Commit af0d6fc has been approved by eddyb

[bors]

🌲 The tree is currently closed for pull requests below priority 5, this pull request will be tested once the tree is reopened