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

Special case repeating fieldless enum variants #104452

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/invalidation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ impl<'cx, 'tcx> InvalidationGenerator<'cx, 'tcx> {
Rvalue::ThreadLocalRef(_) => {}

Rvalue::Use(ref operand)
| Rvalue::Repeat(ref operand, _)
| Rvalue::Repeat(ref operand, _, _)
| Rvalue::UnaryOp(_ /*un_op*/, ref operand)
| Rvalue::Cast(_ /*cast_kind*/, ref operand, _ /*ty*/)
| Rvalue::ShallowInitBox(ref operand, _ /*ty*/) => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1233,7 +1233,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
Rvalue::ThreadLocalRef(_) => {}

Rvalue::Use(ref operand)
| Rvalue::Repeat(ref operand, _)
| Rvalue::Repeat(ref operand, _, _)
| Rvalue::UnaryOp(_ /*un_op*/, ref operand)
| Rvalue::Cast(_ /*cast_kind*/, ref operand, _ /*ty*/)
| Rvalue::ShallowInitBox(ref operand, _ /*ty*/) => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1854,7 +1854,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
self.check_aggregate_rvalue(&body, rvalue, ak, ops, location)
}

Rvalue::Repeat(operand, len) => {
Rvalue::Repeat(operand, len, _) => {
self.check_operand(operand, location);

// If the length cannot be evaluated we must assume that the length can be larger
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_codegen_ssa/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#![feature(int_roundings)]
#![feature(if_let_guard)]
#![feature(never_type)]
#![feature(let_chains)]
#![recursion_limit = "256"]
#![allow(rustc::potential_query_instability)]

Expand Down
11 changes: 10 additions & 1 deletion compiler/rustc_codegen_ssa/src/mir/rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
bx
}

mir::Rvalue::Repeat(ref elem, count) => {
mir::Rvalue::Repeat(ref elem, count, enum_tag_only) => {
let cg_elem = self.codegen_operand(&mut bx, elem);

// Do not generate the loop for zero-sized elements or empty arrays.
Expand All @@ -99,6 +99,15 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
bx.memset(start, v, size, dest.align, MemFlags::empty());
return bx;
}
} else if let OperandValue::Pair(tag, _) = cg_elem.val && enum_tag_only {
let zero = bx.const_usize(0);
let start = dest.project_index(&mut bx, zero).llval;
let size = bx.const_usize(dest.layout.size.bytes());
let v = bx.from_immediate(tag);
if bx.cx().val_ty(v) == bx.cx().type_i8() {
bx.memset(start, v, size, dest.align, MemFlags::empty());
return bx;
}
}

let count =
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/step.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}
}

Repeat(ref operand, _) => {
Repeat(ref operand, _, _) => {
Copy link
Member

@RalfJung RalfJung Nov 15, 2022

Choose a reason for hiding this comment

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

If there is possible UB due to that boolean being wrong, then the interpreter must be adjusted to be able to detect that UB.

There must never be a bit of valid MIR (in the sense of MIR that passes the MIR checker) where the interpreter and codegen behave differently.

let src = self.eval_operand(operand, None)?;
assert!(src.layout.is_sized());
let dest = self.force_allocation(&dest)?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ where
Rvalue::CopyForDeref(place) => in_place::<Q, _>(cx, in_local, place.as_ref()),

Rvalue::Use(operand)
| Rvalue::Repeat(operand, _)
| Rvalue::Repeat(operand, _, _)
| Rvalue::UnaryOp(_, operand)
| Rvalue::Cast(_, operand, _)
| Rvalue::ShallowInitBox(operand, _) => in_operand::<Q, _>(cx, in_local, operand),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/transform/promote_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ impl<'tcx> Validator<'_, 'tcx> {

fn validate_rvalue(&mut self, rvalue: &Rvalue<'tcx>) -> Result<(), Unpromotable> {
match rvalue {
Rvalue::Use(operand) | Rvalue::Repeat(operand, _) => {
Rvalue::Use(operand) | Rvalue::Repeat(operand, _, _) => {
self.validate_operand(operand)?;
}
Rvalue::CopyForDeref(place) => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/transform/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
}
}
}
Rvalue::Repeat(_, _)
Rvalue::Repeat(_, _, _)
| Rvalue::ThreadLocalRef(_)
| Rvalue::AddressOf(_, _)
| Rvalue::NullaryOp(_, _)
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1920,7 +1920,7 @@ impl<'tcx> Rvalue<'tcx> {

Rvalue::Use(_)
| Rvalue::CopyForDeref(_)
| Rvalue::Repeat(_, _)
| Rvalue::Repeat(_, _, _)
| Rvalue::Ref(_, _, _)
| Rvalue::ThreadLocalRef(_)
| Rvalue::AddressOf(_, _)
Expand Down Expand Up @@ -1979,7 +1979,7 @@ impl<'tcx> Debug for Rvalue<'tcx> {

match *self {
Use(ref place) => write!(fmt, "{:?}", place),
Repeat(ref a, b) => {
Repeat(ref a, b, _) => {
write!(fmt, "[{:?}; ", a)?;
pretty_print_const(b, fmt, false)?;
write!(fmt, "]")
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_middle/src/mir/syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1004,10 +1004,13 @@ pub enum Rvalue<'tcx> {
/// This is the cause of a bug in the case where the repetition count is zero because the value
/// is not dropped, see [#74836].
///
/// The third param is true if the value we're repeating is an enum variant
/// with no fields so we can emit a memset.
Copy link
Member

@RalfJung RalfJung Nov 15, 2022

Choose a reason for hiding this comment

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

So what is the behavior of Repeat if that thing is true but the value is not such an enum variant? Is it UB? And what about when the thing is false but it is such an enum variant?

I agree with @scottmcm, encoding this in the syntax of MIR seems wrong. MIR is a programming language and you wouldn't usually expect the syntax of a language to encode redundant information like this.

(Also unnamed booloeans are pretty bad, so if we stick with this design, this should be turned into a struct variant where the fields have names.)

///
/// Corresponds to source code like `[x; 32]`.
///
/// [#74836]: https://github.com/rust-lang/rust/issues/74836
Repeat(Operand<'tcx>, ty::Const<'tcx>),
Repeat(Operand<'tcx>, ty::Const<'tcx>, bool),
Copy link
Member

Choose a reason for hiding this comment

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

It feels weird to need this specific property on RValue? It feels like it could be gotten from looking at the type of the operand when needed. It doesn't seem like it ought to be part of MIR syntax.

(Also, primitives in tuple-variants are often a bad idea, since there's no type hint for what it means.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put the property into the rvalue since the actual variant being written is lost at the point I use the information - we only have a value loaded from the generated temporary (i.e. (i8: %9 = zext i1 %5 to i8))

Copy link
Member

Choose a reason for hiding this comment

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

If this is known at MIR building but not later, one possibility might be to add a NonDivergingIntrinsic::MemSet that you emit instead of the Repeat sometimes.

(That's still an addition to the language that would need a spec, so it's still better to avoid if possible, but it's much lower impact than changing a top-level statement.)


/// Creates a reference of the indicated kind to the place.
///
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/mir/tcx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ impl<'tcx> Rvalue<'tcx> {
{
match *self {
Rvalue::Use(ref operand) => operand.ty(local_decls, tcx),
Rvalue::Repeat(ref operand, count) => {
Rvalue::Repeat(ref operand, count, _) => {
tcx.mk_ty(ty::Array(operand.ty(local_decls, tcx), count))
}
Rvalue::ThreadLocalRef(did) => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/mir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ macro_rules! make_mir_visitor {
self.visit_operand(operand, location);
}

Rvalue::Repeat(value, _) => {
Rvalue::Repeat(value, _, _) => {
self.visit_operand(value, location);
}

Expand Down
21 changes: 13 additions & 8 deletions compiler/rustc_mir_build/src/build/expr/as_rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,21 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
if Some(0) == count.try_eval_usize(this.tcx, this.param_env) {
this.build_zero_repeat(block, value, scope, source_info)
} else {
let value = &this.thir[value];
let enum_tag_only = if let ExprKind::Scope {value: val, ..} = value.kind
&& let ExprKind::Adt(adt) = &this.thir[val].kind
&& adt.adt_def.is_enum() {
// If this is an enum, and this variant has no fields, *and* other
// variants do have fields, store this information. This allows emitting
// a memset, instead of a series of single writes to the tag field
adt.fields.is_empty() && adt.adt_def.all_fields().count() > 0
} else {
false
};
let value_operand = unpack!(
block = this.as_operand(
block,
scope,
&this.thir[value],
None,
NeedsTemporary::No
)
block = this.as_operand(block, scope, value, None, NeedsTemporary::No)
);
block.and(Rvalue::Repeat(value_operand, count))
block.and(Rvalue::Repeat(value_operand, count, enum_tag_only))
}
}
ExprKind::Binary { op, lhs, rhs } => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_dataflow/src/move_paths/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {
match *rvalue {
Rvalue::ThreadLocalRef(_) => {} // not-a-move
Rvalue::Use(ref operand)
| Rvalue::Repeat(ref operand, _)
| Rvalue::Repeat(ref operand, _, _)
| Rvalue::Cast(_, ref operand, _)
| Rvalue::ShallowInitBox(ref operand, _)
| Rvalue::UnaryOp(_, ref operand) => self.gather_operand(operand),
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_mir_transform/src/separate_const_switch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ fn is_likely_const<'tcx>(mut tracked_place: Place<'tcx>, block: &BasicBlockData<
| Rvalue::UnaryOp(_, Operand::Constant(_)) => return true,

// These rvalues make things ambiguous
Rvalue::Repeat(_, _)
Rvalue::Repeat(_, _, _)
| Rvalue::ThreadLocalRef(_)
| Rvalue::Len(_)
| Rvalue::BinaryOp(_, _)
Expand Down Expand Up @@ -282,7 +282,7 @@ fn find_determining_place<'tcx>(
| Rvalue::UnaryOp(_, Operand::Copy(new) | Operand::Move(new))
| Rvalue::CopyForDeref(new)
| Rvalue::Cast(_, Operand::Move(new) | Operand::Copy(new), _)
| Rvalue::Repeat(Operand::Move(new) | Operand::Copy(new), _)
| Rvalue::Repeat(Operand::Move(new) | Operand::Copy(new), _, _)
| Rvalue::Discriminant(new)
=> switch_place = new,

Expand All @@ -297,7 +297,7 @@ fn find_determining_place<'tcx>(
// The following rvalues definitely mean we cannot
// or should not apply this optimization
| Rvalue::Use(Operand::Constant(_))
| Rvalue::Repeat(Operand::Constant(_), _)
| Rvalue::Repeat(Operand::Constant(_), _, _)
| Rvalue::ThreadLocalRef(_)
| Rvalue::AddressOf(_, _)
| Rvalue::NullaryOp(_, _)
Expand Down
19 changes: 19 additions & 0 deletions src/test/codegen/enum-repeat.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// compile-flags: -O

#![crate_type = "lib"]

// CHECK-LABEL: @none_repeat
#[no_mangle]
pub fn none_repeat() -> [Option<u8>; 64] {
// CHECK: call void @llvm.memset
// CHECK-NEXT: ret void
[None; 64]
}

// CHECK-LABEL: @some_repeat
#[no_mangle]
pub fn some_repeat() -> [Option<u8>; 64] {
// CHECK: call void @llvm.memset
// CHECK-NEXT: ret void
[Some(1); 64]
}