Skip to content

Commit d6eb0f5

Browse files
borsbors
committed
Auto merge of rust-lang#122582 - scottmcm:swap-intrinsic-v2, r=oli-obk
Let codegen decide when to `mem::swap` with immediates Making `libcore` decide this is silly; the backend has so much better information about when it's a good idea. Thus this PR introduces a new `typed_swap` intrinsic with a fallback body, and replaces that fallback implementation when swapping immediates or scalar pairs. r? oli-obk Replaces rust-lang#111744, and means we'll never need more libs PRs like rust-lang#111803 or rust-lang#107140
2 parents c3b05c6 + 75d2e5b commit d6eb0f5

File tree

18 files changed

+370
-70
lines changed

18 files changed

+370
-70
lines changed

‎compiler/rustc_codegen_ssa/src/mir/intrinsic.rs

+24
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ use crate::traits::*;
99
use crate::MemFlags;
1010

1111
use rustc_middle::ty::{self, Ty, TyCtxt};
12+
use rustc_session::config::OptLevel;
1213
use rustc_span::{sym, Span};
1314
use rustc_target::abi::{
1415
call::{FnAbi, PassMode},
@@ -75,6 +76,29 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
7576
let name = bx.tcx().item_name(def_id);
7677
let name_str = name.as_str();
7778

79+
// If we're swapping something that's *not* an `OperandValue::Ref`,
80+
// then we can do it directly and avoid the alloca.
81+
// Otherwise, we'll let the fallback MIR body take care of it.
82+
if let sym::typed_swap = name {
83+
let pointee_ty = fn_args.type_at(0);
84+
let pointee_layout = bx.layout_of(pointee_ty);
85+
if !bx.is_backend_ref(pointee_layout)
86+
// But if we're not going to optimize, trying to use the fallback
87+
// body just makes things worse, so don't bother.
88+
|| bx.sess().opts.optimize == OptLevel::No
89+
// NOTE(eddyb) SPIR-V's Logical addressing model doesn't allow for arbitrary
90+
// reinterpretation of values as (chunkable) byte arrays, and the loop in the
91+
// block optimization in `ptr::swap_nonoverlapping` is hard to rewrite back
92+
// into the (unoptimized) direct swapping implementation, so we disable it.
93+
|| bx.sess().target.arch == "spirv"
94+
{
95+
let x_place = PlaceRef::new_sized(args[0].immediate(), pointee_layout);
96+
let y_place = PlaceRef::new_sized(args[1].immediate(), pointee_layout);
97+
bx.typed_place_swap(x_place, y_place);
98+
return Ok(());
99+
}
100+
}
101+
78102
let llret_ty = bx.backend_type(bx.layout_of(ret_ty));
79103
let result = PlaceRef::new_sized(llresult, fn_abi.ret.layout);
80104

‎compiler/rustc_codegen_ssa/src/traits/builder.rs

+52-2
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,24 @@
11
use super::abi::AbiBuilderMethods;
22
use super::asm::AsmBuilderMethods;
3+
use super::consts::ConstMethods;
34
use super::coverageinfo::CoverageInfoBuilderMethods;
45
use super::debuginfo::DebugInfoBuilderMethods;
56
use super::intrinsic::IntrinsicCallMethods;
67
use super::misc::MiscMethods;
7-
use super::type_::{ArgAbiMethods, BaseTypeMethods};
8+
use super::type_::{ArgAbiMethods, BaseTypeMethods, LayoutTypeMethods};
89
use super::{HasCodegen, StaticBuilderMethods};
910

1011
use crate::common::{
1112
AtomicOrdering, AtomicRmwBinOp, IntPredicate, RealPredicate, SynchronizationScope, TypeKind,
1213
};
13-
use crate::mir::operand::OperandRef;
14+
use crate::mir::operand::{OperandRef, OperandValue};
1415
use crate::mir::place::PlaceRef;
1516
use crate::MemFlags;
1617

1718
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrs;
1819
use rustc_middle::ty::layout::{HasParamEnv, TyAndLayout};
1920
use rustc_middle::ty::Ty;
21+
use rustc_session::config::OptLevel;
2022
use rustc_span::Span;
2123
use rustc_target::abi::call::FnAbi;
2224
use rustc_target::abi::{Abi, Align, Scalar, Size, WrappingRange};
@@ -267,6 +269,54 @@ pub trait BuilderMethods<'a, 'tcx>:
267269
flags: MemFlags,
268270
);
269271

272+
/// *Typed* copy for non-overlapping places.
273+
///
274+
/// Has a default implementation in terms of `memcpy`, but specific backends
275+
/// can override to do something smarter if possible.
276+
///
277+
/// (For example, typed load-stores with alias metadata.)
278+
fn typed_place_copy(
279+
&mut self,
280+
dst: PlaceRef<'tcx, Self::Value>,
281+
src: PlaceRef<'tcx, Self::Value>,
282+
) {
283+
debug_assert!(src.llextra.is_none());
284+
debug_assert!(dst.llextra.is_none());
285+
debug_assert_eq!(dst.layout.size, src.layout.size);
286+
if self.sess().opts.optimize == OptLevel::No && self.is_backend_immediate(dst.layout) {
287+
// If we're not optimizing, the aliasing information from `memcpy`
288+
// isn't useful, so just load-store the value for smaller code.
289+
let temp = self.load_operand(src);
290+
temp.val.store(self, dst);
291+
} else if !dst.layout.is_zst() {
292+
let bytes = self.const_usize(dst.layout.size.bytes());
293+
self.memcpy(dst.llval, dst.align, src.llval, src.align, bytes, MemFlags::empty());
294+
}
295+
}
296+
297+
/// *Typed* swap for non-overlapping places.
298+
///
299+
/// Avoids `alloca`s for Immediates and ScalarPairs.
300+
///
301+
/// FIXME: Maybe do something smarter for Ref types too?
302+
/// For now, the `typed_swap` intrinsic just doesn't call this for those
303+
/// cases (in non-debug), preferring the fallback body instead.
304+
fn typed_place_swap(
305+
&mut self,
306+
left: PlaceRef<'tcx, Self::Value>,
307+
right: PlaceRef<'tcx, Self::Value>,
308+
) {
309+
let mut temp = self.load_operand(left);
310+
if let OperandValue::Ref(..) = temp.val {
311+
// The SSA value isn't stand-alone, so we need to copy it elsewhere
312+
let alloca = PlaceRef::alloca(self, left.layout);
313+
self.typed_place_copy(alloca, left);
314+
temp = self.load_operand(alloca);
315+
}
316+
self.typed_place_copy(left, right);
317+
temp.val.store(self, right);
318+
}
319+
270320
fn select(
271321
&mut self,
272322
cond: Self::Value,

‎compiler/rustc_codegen_ssa/src/traits/type_.rs

+14
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,20 @@ pub trait LayoutTypeMethods<'tcx>: Backend<'tcx> {
120120
immediate: bool,
121121
) -> Self::Type;
122122

123+
/// A type that produces an [`OperandValue::Ref`] when loaded.
124+
///
125+
/// AKA one that's not a ZST, not `is_backend_immediate`, and
126+
/// not `is_backend_scalar_pair`. For such a type, a
127+
/// [`load_operand`] doesn't actually `load` anything.
128+
///
129+
/// [`OperandValue::Ref`]: crate::mir::operand::OperandValue::Ref
130+
/// [`load_operand`]: super::BuilderMethods::load_operand
131+
fn is_backend_ref(&self, layout: TyAndLayout<'tcx>) -> bool {
132+
!(layout.is_zst()
133+
|| self.is_backend_immediate(layout)
134+
|| self.is_backend_scalar_pair(layout))
135+
}
136+
123137
/// A type that can be used in a [`super::BuilderMethods::load`] +
124138
/// [`super::BuilderMethods::store`] pair to implement a *typed* copy,
125139
/// such as a MIR `*_0 = *_1`.

‎compiler/rustc_const_eval/src/interpret/intrinsics.rs

+23-2
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,8 @@ use rustc_span::symbol::{sym, Symbol};
2121
use rustc_target::abi::Size;
2222

2323
use super::{
24-
util::ensure_monomorphic_enough, CheckInAllocMsg, ImmTy, InterpCx, MPlaceTy, Machine, OpTy,
25-
Pointer,
24+
memory::MemoryKind, util::ensure_monomorphic_enough, CheckInAllocMsg, ImmTy, InterpCx,
25+
MPlaceTy, Machine, OpTy, Pointer,
2626
};
2727

2828
use crate::fluent_generated as fluent;
@@ -414,6 +414,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
414414
let result = self.raw_eq_intrinsic(&args[0], &args[1])?;
415415
self.write_scalar(result, dest)?;
416416
}
417+
sym::typed_swap => {
418+
self.typed_swap_intrinsic(&args[0], &args[1])?;
419+
}
417420

418421
sym::vtable_size => {
419422
let ptr = self.read_pointer(&args[0])?;
@@ -607,6 +610,24 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
607610
self.mem_copy(src, dst, size, nonoverlapping)
608611
}
609612

613+
/// Does a *typed* swap of `*left` and `*right`.
614+
fn typed_swap_intrinsic(
615+
&mut self,
616+
left: &OpTy<'tcx, <M as Machine<'mir, 'tcx>>::Provenance>,
617+
right: &OpTy<'tcx, <M as Machine<'mir, 'tcx>>::Provenance>,
618+
) -> InterpResult<'tcx> {
619+
let left = self.deref_pointer(left)?;
620+
let right = self.deref_pointer(right)?;
621+
debug_assert_eq!(left.layout, right.layout);
622+
let kind = MemoryKind::Stack;
623+
let temp = self.allocate(left.layout, kind)?;
624+
self.copy_op(&left, &temp)?;
625+
self.copy_op(&right, &left)?;
626+
self.copy_op(&temp, &right)?;
627+
self.deallocate_ptr(temp.ptr(), None, kind)?;
628+
Ok(())
629+
}
630+
610631
pub(crate) fn write_bytes_intrinsic(
611632
&mut self,
612633
dst: &OpTy<'tcx, <M as Machine<'mir, 'tcx>>::Provenance>,

‎compiler/rustc_hir_analysis/src/check/intrinsic.rs

+2
Original file line numberDiff line numberDiff line change
@@ -484,6 +484,8 @@ pub fn check_intrinsic_type(
484484
(1, 0, vec![Ty::new_mut_ptr(tcx, param(0)), param(0)], Ty::new_unit(tcx))
485485
}
486486

487+
sym::typed_swap => (1, 1, vec![Ty::new_mut_ptr(tcx, param(0)); 2], Ty::new_unit(tcx)),
488+
487489
sym::discriminant_value => {
488490
let assoc_items = tcx.associated_item_def_ids(
489491
tcx.require_lang_item(hir::LangItem::DiscriminantKind, None),

‎compiler/rustc_span/src/symbol.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1836,6 +1836,7 @@ symbols! {
18361836
type_macros,
18371837
type_name,
18381838
type_privacy_lints,
1839+
typed_swap,
18391840
u128,
18401841
u128_legacy_const_max,
18411842
u128_legacy_const_min,

‎library/core/src/intrinsics.rs

+22
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@
6666
use crate::marker::DiscriminantKind;
6767
use crate::marker::Tuple;
6868
use crate::mem::align_of;
69+
use crate::ptr;
6970

7071
pub mod mir;
7172
pub mod simd;
@@ -2638,6 +2639,27 @@ pub const fn is_val_statically_known<T: Copy>(_arg: T) -> bool {
26382639
false
26392640
}
26402641

2642+
/// Non-overlapping *typed* swap of a single value.
2643+
///
2644+
/// The codegen backends will replace this with a better implementation when
2645+
/// `T` is a simple type that can be loaded and stored as an immediate.
2646+
///
2647+
/// The stabilized form of this intrinsic is [`crate::mem::swap`].
2648+
///
2649+
/// # Safety
2650+
///
2651+
/// `x` and `y` are readable and writable as `T`, and non-overlapping.
2652+
#[rustc_nounwind]
2653+
#[inline]
2654+
#[cfg_attr(not(bootstrap), rustc_intrinsic)]
2655+
// This has fallback `const fn` MIR, so shouldn't need stability, see #122652
2656+
#[rustc_const_unstable(feature = "const_typed_swap", issue = "none")]
2657+
pub const unsafe fn typed_swap<T>(x: *mut T, y: *mut T) {
2658+
// SAFETY: The caller provided single non-overlapping items behind
2659+
// pointers, so swapping them with `count: 1` is fine.
2660+
unsafe { ptr::swap_nonoverlapping(x, y, 1) };
2661+
}
2662+
26412663
/// Returns whether we should check for library UB. This evaluate to the value of `cfg!(debug_assertions)`
26422664
/// during monomorphization.
26432665
///

‎library/core/src/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,7 @@
170170
#![feature(const_try)]
171171
#![feature(const_type_id)]
172172
#![feature(const_type_name)]
173+
#![feature(const_typed_swap)]
173174
#![feature(const_unicode_case_lookup)]
174175
#![feature(const_unsafecell_get_mut)]
175176
#![feature(const_waker)]

‎library/core/src/mem/mod.rs

+3-57
Original file line numberDiff line numberDiff line change
@@ -726,63 +726,9 @@ pub unsafe fn uninitialized<T>() -> T {
726726
#[rustc_const_unstable(feature = "const_swap", issue = "83163")]
727727
#[rustc_diagnostic_item = "mem_swap"]
728728
pub const fn swap<T>(x: &mut T, y: &mut T) {
729-
// NOTE(eddyb) SPIR-V's Logical addressing model doesn't allow for arbitrary
730-
// reinterpretation of values as (chunkable) byte arrays, and the loop in the
731-
// block optimization in `swap_slice` is hard to rewrite back
732-
// into the (unoptimized) direct swapping implementation, so we disable it.
733-
#[cfg(not(any(target_arch = "spirv")))]
734-
{
735-
// For types that are larger multiples of their alignment, the simple way
736-
// tends to copy the whole thing to stack rather than doing it one part
737-
// at a time, so instead treat them as one-element slices and piggy-back
738-
// the slice optimizations that will split up the swaps.
739-
if const { size_of::<T>() / align_of::<T>() > 2 } {
740-
// SAFETY: exclusive references always point to one non-overlapping
741-
// element and are non-null and properly aligned.
742-
return unsafe { ptr::swap_nonoverlapping(x, y, 1) };
743-
}
744-
}
745-
746-
// If a scalar consists of just a small number of alignment units, let
747-
// the codegen just swap those pieces directly, as it's likely just a
748-
// few instructions and anything else is probably overcomplicated.
749-
//
750-
// Most importantly, this covers primitives and simd types that tend to
751-
// have size=align where doing anything else can be a pessimization.
752-
// (This will also be used for ZSTs, though any solution works for them.)
753-
swap_simple(x, y);
754-
}
755-
756-
/// Same as [`swap`] semantically, but always uses the simple implementation.
757-
///
758-
/// Used elsewhere in `mem` and `ptr` at the bottom layer of calls.
759-
#[rustc_const_unstable(feature = "const_swap", issue = "83163")]
760-
#[inline]
761-
pub(crate) const fn swap_simple<T>(x: &mut T, y: &mut T) {
762-
// We arrange for this to typically be called with small types,
763-
// so this reads-and-writes approach is actually better than using
764-
// copy_nonoverlapping as it easily puts things in LLVM registers
765-
// directly and doesn't end up inlining allocas.
766-
// And LLVM actually optimizes it to 3×memcpy if called with
767-
// a type larger than it's willing to keep in a register.
768-
// Having typed reads and writes in MIR here is also good as
769-
// it lets Miri and CTFE understand them better, including things
770-
// like enforcing type validity for them.
771-
// Importantly, read+copy_nonoverlapping+write introduces confusing
772-
// asymmetry to the behaviour where one value went through read+write
773-
// whereas the other was copied over by the intrinsic (see #94371).
774-
// Furthermore, using only read+write here benefits limited backends
775-
// such as SPIR-V that work on an underlying *typed* view of memory,
776-
// and thus have trouble with Rust's untyped memory operations.
777-
778-
// SAFETY: exclusive references are always valid to read/write,
779-
// including being aligned, and nothing here panics so it's drop-safe.
780-
unsafe {
781-
let a = ptr::read(x);
782-
let b = ptr::read(y);
783-
ptr::write(x, b);
784-
ptr::write(y, a);
785-
}
729+
// SAFETY: `&mut` guarantees these are typed readable and writable
730+
// as well as non-overlapping.
731+
unsafe { intrinsics::typed_swap(x, y) }
786732
}
787733

788734
/// Replaces `dest` with the default value of `T`, returning the previous `dest` value.

‎library/core/src/ptr/mod.rs

+18-3
Original file line numberDiff line numberDiff line change
@@ -1062,11 +1062,26 @@ const unsafe fn swap_nonoverlapping_simple_untyped<T>(x: *mut T, y: *mut T, coun
10621062
let mut i = 0;
10631063
while i < count {
10641064
// SAFETY: By precondition, `i` is in-bounds because it's below `n`
1065-
let x = unsafe { &mut *x.add(i) };
1065+
let x = unsafe { x.add(i) };
10661066
// SAFETY: By precondition, `i` is in-bounds because it's below `n`
10671067
// and it's distinct from `x` since the ranges are non-overlapping
1068-
let y = unsafe { &mut *y.add(i) };
1069-
mem::swap_simple::<MaybeUninit<T>>(x, y);
1068+
let y = unsafe { y.add(i) };
1069+
1070+
// If we end up here, it's because we're using a simple type -- like
1071+
// a small power-of-two-sized thing -- or a special type with particularly
1072+
// large alignment, particularly SIMD types.
1073+
// Thus we're fine just reading-and-writing it, as either it's small
1074+
// and that works well anyway or it's special and the type's author
1075+
// presumably wanted things to be done in the larger chunk.
1076+
1077+
// SAFETY: we're only ever given pointers that are valid to read/write,
1078+
// including being aligned, and nothing here panics so it's drop-safe.
1079+
unsafe {
1080+
let a: MaybeUninit<T> = read(x);
1081+
let b: MaybeUninit<T> = read(y);
1082+
write(x, b);
1083+
write(y, a);
1084+
}
10701085

10711086
i += 1;
10721087
}

‎src/tools/miri/tests/fail/intrinsics/typed-swap-invalid-array.rs

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
#![feature(core_intrinsics)]
2+
#![feature(rustc_attrs)]
3+
4+
use std::intrinsics::typed_swap;
5+
use std::ptr::addr_of_mut;
6+
7+
fn invalid_array() {
8+
let mut a = [1_u8; 100];
9+
let mut b = [2_u8; 100];
10+
unsafe {
11+
let a = addr_of_mut!(a).cast::<[bool; 100]>();
12+
let b = addr_of_mut!(b).cast::<[bool; 100]>();
13+
typed_swap(a, b); //~ERROR: constructing invalid value
14+
}
15+
}
16+
17+
fn main() {
18+
invalid_array();
19+
}

‎src/tools/miri/tests/fail/intrinsics/typed-swap-invalid-array.stderr

+20
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
error: Undefined Behavior: constructing invalid value at [0]: encountered 0x02, but expected a boolean
2+
--> $DIR/typed-swap-invalid-array.rs:LL:CC
3+
|
4+
LL | typed_swap(a, b);
5+
| ^^^^^^^^^^^^^^^^ constructing invalid value at [0]: encountered 0x02, but expected a boolean
6+
|
7+
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
8+
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
9+
= note: BACKTRACE:
10+
= note: inside `invalid_array` at $DIR/typed-swap-invalid-array.rs:LL:CC
11+
note: inside `main`
12+
--> $DIR/typed-swap-invalid-array.rs:LL:CC
13+
|
14+
LL | invalid_array();
15+
| ^^^^^^^^^^^^^^^
16+
17+
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
18+
19+
error: aborting due to 1 previous error
20+

0 commit comments

Comments
 (0)