Skip to content

Commit abd7739

Browse files
borsbors
committed
Auto merge of #138582 - scottmcm:option-ssa-2, r=<try>
Don't require `alloca`s for consuming simple enums Well, 4 months later I'm finally back to this. For example, if you pass an `Option<u32>` to a function, don't immediately write it to an `alloca` then read it again.
2 parents ca98d4d + bc074f1 commit abd7739

File tree

13 files changed

+318
-192
lines changed

13 files changed

+318
-192
lines changed

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

Lines changed: 52 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use rustc_index::bit_set::DenseBitSet;
66
use rustc_index::{IndexSlice, IndexVec};
77
use rustc_middle::mir::visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor};
88
use rustc_middle::mir::{self, DefLocation, Location, TerminatorKind, traversal};
9-
use rustc_middle::ty::layout::{HasTyCtxt, LayoutOf};
9+
use rustc_middle::ty::layout::LayoutOf;
1010
use rustc_middle::{bug, span_bug};
1111
use tracing::debug;
1212

@@ -99,63 +99,64 @@ impl<'a, 'b, 'tcx, Bx: BuilderMethods<'b, 'tcx>> LocalAnalyzer<'a, 'b, 'tcx, Bx>
9999
context: PlaceContext,
100100
location: Location,
101101
) {
102-
let cx = self.fx.cx;
102+
const COPY_CONTEXT: PlaceContext =
103+
PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy);
104+
105+
// `PlaceElem::Index` is the only variant that can mention other `Local`s,
106+
// so check for those up-front before any potential short-circuits.
107+
for elem in place_ref.projection {
108+
if let mir::PlaceElem::Index(index_local) = *elem {
109+
self.visit_local(index_local, COPY_CONTEXT, location);
110+
}
111+
}
103112

104-
if let Some((place_base, elem)) = place_ref.last_projection() {
105-
let mut base_context = if context.is_mutating_use() {
106-
PlaceContext::MutatingUse(MutatingUseContext::Projection)
107-
} else {
108-
PlaceContext::NonMutatingUse(NonMutatingUseContext::Projection)
109-
};
113+
// If our local is already memory, nothing can make it *more* memory
114+
// so we don't need to bother checking the projections further.
115+
if self.locals[place_ref.local] == LocalKind::Memory {
116+
return;
117+
}
110118

111-
// Allow uses of projections that are ZSTs or from scalar fields.
112-
let is_consume = matches!(
113-
context,
114-
PlaceContext::NonMutatingUse(
115-
NonMutatingUseContext::Copy | NonMutatingUseContext::Move,
116-
)
117-
);
118-
if is_consume {
119-
let base_ty = place_base.ty(self.fx.mir, cx.tcx());
120-
let base_ty = self.fx.monomorphize(base_ty);
121-
122-
// ZSTs don't require any actual memory access.
123-
let elem_ty = base_ty.projection_ty(cx.tcx(), self.fx.monomorphize(elem)).ty;
124-
let span = self.fx.mir.local_decls[place_ref.local].source_info.span;
125-
if cx.spanned_layout_of(elem_ty, span).is_zst() {
126-
return;
127-
}
119+
if place_ref.is_indirect_first_projection() {
120+
// If this starts with a `Deref`, we only need to record a read of the
121+
// pointer being dereferenced, as all the subsequent projections are
122+
// working on a place which is always supported. (And because we're
123+
// looking at codegen MIR, it can only happen as the first projection.)
124+
self.visit_local(place_ref.local, COPY_CONTEXT, location);
125+
return;
126+
}
128127

129-
if let mir::ProjectionElem::Field(..) = elem {
130-
let layout = cx.spanned_layout_of(base_ty.ty, span);
131-
if cx.is_backend_immediate(layout) || cx.is_backend_scalar_pair(layout) {
132-
// Recurse with the same context, instead of `Projection`,
133-
// potentially stopping at non-operand projections,
134-
// which would trigger `not_ssa` on locals.
135-
base_context = context;
136-
}
137-
}
138-
}
128+
if !place_ref.projection.is_empty() && context.is_mutating_use() {
129+
// If it's a mutating use it doesn't matter what the projections are,
130+
// if there are *any* then we need a place to write. (For example,
131+
// `_1 = Foo()` works in SSA but `_2.0 = Foo()` does not.)
132+
let mut_projection = PlaceContext::MutatingUse(MutatingUseContext::Projection);
133+
self.visit_local(place_ref.local, mut_projection, location);
134+
return;
135+
}
139136

140-
if let mir::ProjectionElem::Deref = elem {
141-
// Deref projections typically only read the pointer.
142-
base_context = PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy);
143-
}
137+
for elem in place_ref.projection {
138+
// Scan through to ensure the only projections are those which
139+
// `FunctionCx::maybe_codegen_consume_direct` can handle.
140+
match elem {
141+
mir::PlaceElem::Field(..) | mir::PlaceElem::Downcast(..) => {}
142+
143+
mir::PlaceElem::Index(..)
144+
| mir::PlaceElem::ConstantIndex { .. }
145+
| mir::PlaceElem::Subslice { .. }
146+
| mir::PlaceElem::OpaqueCast(..)
147+
| mir::PlaceElem::UnwrapUnsafeBinder(..)
148+
| mir::PlaceElem::Subtype(..) => {
149+
self.locals[place_ref.local] = LocalKind::Memory;
150+
return;
151+
}
144152

145-
self.process_place(&place_base, base_context, location);
146-
// HACK(eddyb) this emulates the old `visit_projection_elem`, this
147-
// entire `visit_place`-like `process_place` method should be rewritten,
148-
// now that we have moved to the "slice of projections" representation.
149-
if let mir::ProjectionElem::Index(local) = elem {
150-
self.visit_local(
151-
local,
152-
PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy),
153-
location,
154-
);
153+
mir::PlaceElem::Deref => bug!("Deref after first position"),
155154
}
156-
} else {
157-
self.visit_local(place_ref.local, context, location);
158155
}
156+
157+
// Even with supported projections, we still need to have `visit_local`
158+
// check for things that can't be done in SSA (like `SharedBorrow`).
159+
self.visit_local(place_ref.local, context, location);
159160
}
160161
}
161162

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ use tracing::{debug, warn};
1212
use crate::mir::{FunctionCx, LocalRef};
1313
use crate::traits::BuilderMethods;
1414

15+
#[derive(Debug)]
1516
pub(super) struct Locals<'tcx, V> {
1617
values: IndexVec<mir::Local, LocalRef<'tcx, V>>,
1718
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
136136
}
137137
}
138138

139+
#[derive(Debug)]
139140
enum LocalRef<'tcx, V> {
140141
Place(PlaceRef<'tcx, V>),
141142
/// `UnsizedPlace(p)`: `p` itself is a thin pointer (indirect place).

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

Lines changed: 30 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ pub struct OperandRef<'tcx, V> {
126126
pub layout: TyAndLayout<'tcx>,
127127
}
128128

129-
impl<V: CodegenObject> fmt::Debug for OperandRef<'_, V> {
129+
impl<V: fmt::Debug> fmt::Debug for OperandRef<'_, V> {
130130
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
131131
write!(f, "OperandRef({:?} @ {:?})", self.val, self.layout)
132132
}
@@ -361,13 +361,17 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
361361
let (in_scalar, imm) = match (self.val, self.layout.backend_repr) {
362362
// Extract a scalar component from a pair.
363363
(OperandValue::Pair(a_llval, b_llval), BackendRepr::ScalarPair(a, b)) => {
364-
if offset.bytes() == 0 {
364+
// This needs to look at `offset`, rather than `i`, because
365+
// for a type like `Option<u32>`, the first thing in the pair
366+
// is the tag, so `(_2 as Some).0` needs to read the *second*
367+
// thing in the pair despite it being "field zero".
368+
if offset == Size::ZERO {
365369
assert_eq!(field.size, a.size(bx.cx()));
366-
(Some(a), a_llval)
370+
(a, a_llval)
367371
} else {
368372
assert_eq!(offset, a.size(bx.cx()).align_to(b.align(bx.cx()).abi));
369373
assert_eq!(field.size, b.size(bx.cx()));
370-
(Some(b), b_llval)
374+
(b, b_llval)
371375
}
372376
}
373377

@@ -378,23 +382,12 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
378382
OperandValue::Immediate(match field.backend_repr {
379383
BackendRepr::SimdVector { .. } => imm,
380384
BackendRepr::Scalar(out_scalar) => {
381-
let Some(in_scalar) = in_scalar else {
382-
span_bug!(
383-
fx.mir.span,
384-
"OperandRef::extract_field({:?}): missing input scalar for output scalar",
385-
self
386-
)
387-
};
388-
if in_scalar != out_scalar {
389-
// If the backend and backend_immediate types might differ,
390-
// flip back to the backend type then to the new immediate.
391-
// This avoids nop truncations, but still handles things like
392-
// Bools in union fields needs to be truncated.
393-
let backend = bx.from_immediate(imm);
394-
bx.to_immediate_scalar(backend, out_scalar)
395-
} else {
396-
imm
397-
}
385+
// For a type like `Result<usize, &u32>` the layout is `Pair(i64, ptr)`.
386+
// But if we're reading the `Ok` payload, we need to turn that `ptr`
387+
// back into an integer. To avoid repeating logic we do that by
388+
// calling the transmute code, which is legal thanks to the size
389+
// assert we did when pulling it out of the pair.
390+
transmute_scalar(bx, imm, in_scalar, out_scalar)
398391
}
399392
BackendRepr::ScalarPair(_, _) | BackendRepr::Memory { .. } => bug!(),
400393
})
@@ -852,7 +845,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
852845
LocalRef::Operand(mut o) => {
853846
// Moves out of scalar and scalar pair fields are trivial.
854847
for elem in place_ref.projection.iter() {
855-
match elem {
848+
match *elem {
856849
mir::ProjectionElem::Field(f, _) => {
857850
assert!(
858851
!o.layout.ty.is_any_ptr(),
@@ -861,17 +854,21 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
861854
);
862855
o = o.extract_field(self, bx, f.index());
863856
}
864-
mir::ProjectionElem::Index(_)
865-
| mir::ProjectionElem::ConstantIndex { .. } => {
866-
// ZSTs don't require any actual memory access.
867-
// FIXME(eddyb) deduplicate this with the identical
868-
// checks in `codegen_consume` and `extract_field`.
869-
let elem = o.layout.field(bx.cx(), 0);
870-
if elem.is_zst() {
871-
o = OperandRef::zero_sized(elem);
872-
} else {
873-
return None;
874-
}
857+
mir::ProjectionElem::Downcast(_sym, variant_idx) => {
858+
let layout = o.layout.for_variant(bx.cx(), variant_idx);
859+
let val = match layout.backend_repr {
860+
// The transmute here handles cases like `Result<bool, u8>`
861+
// where the immediate values need to change for
862+
// the specific types in the cast-to variant.
863+
BackendRepr::Scalar(..) | BackendRepr::ScalarPair(..) => {
864+
self.codegen_transmute_operand(bx, o, layout)
865+
}
866+
BackendRepr::SimdVector { .. } | BackendRepr::Memory { .. } => {
867+
o.val
868+
}
869+
};
870+
871+
o = OperandRef { val, layout };
875872
}
876873
_ => return None,
877874
}

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

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1056,11 +1056,10 @@ pub(super) fn transmute_scalar<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
10561056
to_scalar: abi::Scalar,
10571057
) -> Bx::Value {
10581058
assert_eq!(from_scalar.size(bx.cx()), to_scalar.size(bx.cx()));
1059-
let imm_ty = bx.cx().val_ty(imm);
1060-
assert_ne!(
1061-
bx.cx().type_kind(imm_ty),
1059+
debug_assert_ne!(
1060+
bx.cx().type_kind(bx.cx().val_ty(imm)),
10621061
TypeKind::Vector,
1063-
"Vector type {imm_ty:?} not allowed in transmute_scalar {from_scalar:?} -> {to_scalar:?}"
1062+
"Vector type {imm:?} not allowed in transmute_scalar {from_scalar:?} -> {to_scalar:?}"
10641063
);
10651064

10661065
// While optimizations will remove no-op transmutes, they might still be
@@ -1086,7 +1085,9 @@ pub(super) fn transmute_scalar<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
10861085
// That said, last time we tried removing this, it didn't actually help
10871086
// the rustc-perf results, so might as well keep doing it
10881087
// <https://github.com/rust-lang/rust/pull/135610#issuecomment-2599275182>
1089-
assume_scalar_range(bx, imm, from_scalar, from_backend_ty);
1088+
//
1089+
// EXPERIMENT: What if we skip it for perf?
1090+
// assume_scalar_range(bx, imm, from_scalar, from_backend_ty);
10901091

10911092
imm = match (from_scalar.primitive(), to_scalar.primitive()) {
10921093
(Int(..) | Float(_), Int(..) | Float(_)) => bx.bitcast(imm, to_backend_ty),
@@ -1109,12 +1110,15 @@ pub(super) fn transmute_scalar<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
11091110

11101111
debug_assert_eq!(bx.cx().val_ty(imm), to_backend_ty);
11111112

1112-
// This `assume` remains important for cases like (a conceptual)
1113-
// transmute::<u32, NonZeroU32>(x) == 0
1114-
// since it's never passed to something with parameter metadata (especially
1115-
// after MIR inlining) so the only way to tell the backend about the
1116-
// constraint that the `transmute` introduced is to `assume` it.
1117-
assume_scalar_range(bx, imm, to_scalar, to_backend_ty);
1113+
// For bool we don't need the assert since `to_immediate_scalar` covers it.
1114+
if !to_scalar.is_bool() {
1115+
// This `assume` remains important for cases like (a conceptual)
1116+
// transmute::<u32, NonZeroU32>(x) == 0
1117+
// since it's never passed to something with parameter metadata (especially
1118+
// after MIR inlining) so the only way to tell the backend about the
1119+
// constraint that the `transmute` introduced is to `assume` it.
1120+
assume_scalar_range(bx, imm, to_scalar, to_backend_ty);
1121+
}
11181122

11191123
imm = bx.to_immediate_scalar(imm, to_scalar);
11201124
imm

‎tests/codegen/common_prim_int_ptr.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,13 @@ pub unsafe fn extract_int(x: Result<usize, Box<()>>) -> usize {
4040
}
4141

4242
// CHECK-LABEL: @extract_box
43-
// CHECK-SAME: (i{{[0-9]+}} {{[^%]+}} [[DISCRIMINANT:%[0-9]+]], ptr {{[^%]+}} [[PAYLOAD:%[0-9]+]])
43+
// CHECK-SAME: (i{{[0-9]+}} {{[^%]+}} [[DISCRIMINANT:%x.0]], ptr {{[^%]+}} [[PAYLOAD:%x.1]])
4444
#[no_mangle]
4545
pub unsafe fn extract_box(x: Result<usize, Box<i32>>) -> Box<i32> {
46+
// CHECK: [[NOT_OK:%.+]] = icmp ne i{{[0-9]+}} [[DISCRIMINANT]], 0
47+
// CHECK: call void @llvm.assume(i1 [[NOT_OK]])
48+
// CHECK: [[NOT_NULL:%.+]] = icmp ne ptr [[PAYLOAD]], null
49+
// CHECK: call void @llvm.assume(i1 [[NOT_NULL]])
4650
// CHECK: ret ptr [[PAYLOAD]]
4751
match x {
4852
Ok(_) => std::intrinsics::unreachable(),

0 commit comments

Comments
 (0)