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

[mir-opt] simplify Repeats that don't actually repeat the operand #135322

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
13 changes: 13 additions & 0 deletions compiler/rustc_mir_transform/src/instsimplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ impl<'tcx> crate::MirPass<'tcx> for InstSimplify {
ctx.simplify_ref_deref(rvalue);
ctx.simplify_ptr_aggregate(rvalue);
ctx.simplify_cast(rvalue);
ctx.simplify_repeat_once(rvalue);
}
_ => {}
}
Expand Down Expand Up @@ -177,6 +178,18 @@ impl<'tcx> InstSimplifyContext<'_, 'tcx> {
}
}

/// Simplify `[x; 1]` to just `[x]`.
fn simplify_repeat_once(&self, rvalue: &mut Rvalue<'tcx>) {
if let Rvalue::Repeat(operand, count) = rvalue
&& let Some(1) = count.try_to_target_usize(self.tcx)
{
*rvalue = Rvalue::Aggregate(
Box::new(AggregateKind::Array(operand.ty(self.local_decls, self.tcx))),
[operand.clone()].into(),
);
}
}

fn simplify_primitive_clone(
&self,
terminator: &mut Terminator<'tcx>,
Expand Down
28 changes: 17 additions & 11 deletions compiler/rustc_mir_transform/src/remove_zsts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,31 +36,37 @@ struct Replacer<'a, 'tcx> {
}

/// A cheap, approximate check to avoid unnecessary `layout_of` calls.
fn maybe_zst(ty: Ty<'_>) -> bool {
///
/// `Some(true)` is definitely ZST; `Some(false)` is definitely *not* ZST.
///
/// `None` may or may not be, and must check `layout_of` to be sure.
fn trivially_zst<'tcx>(ty: Ty<'tcx>, tcx: TyCtxt<'tcx>) -> Option<bool> {
match ty.kind() {
// definitely ZST
ty::FnDef(..) | ty::Never => Some(true),
ty::Tuple(fields) if fields.is_empty() => Some(true),
ty::Array(_ty, len) if let Some(0) = len.try_to_target_usize(tcx) => Some(true),
// maybe ZST (could be more precise)
ty::Adt(..)
| ty::Array(..)
| ty::Closure(..)
| ty::CoroutineClosure(..)
| ty::Tuple(..)
| ty::Alias(ty::Opaque, ..) => true,
// definitely ZST
ty::FnDef(..) | ty::Never => true,
| ty::Alias(ty::Opaque, ..) => None,
// unreachable or can't be ZST
_ => false,
_ => Some(false),
Copy link
Contributor

Choose a reason for hiding this comment

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

exhaustively match here. This branch must not have false positives given that it means "definitely not"

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Spurious false is benign in how it's actually used, but that's not what the comments say, so I should re-work it. Maybe just have the wildcard be _ => None, since that's more obviously fine, and add the known-false cases explicitly.

}
}

impl<'tcx> Replacer<'_, 'tcx> {
fn known_to_be_zst(&self, ty: Ty<'tcx>) -> bool {
if !maybe_zst(ty) {
return false;
if let Some(is_zst) = trivially_zst(ty, self.tcx) {
is_zst
} else {
self.tcx
.layout_of(self.typing_env.as_query_input(ty))
.is_ok_and(|layout| layout.is_zst())
}
let Ok(layout) = self.tcx.layout_of(self.typing_env.as_query_input(ty)) else {
return false;
};
layout.is_zst()
}

fn make_zst(&self, ty: Ty<'tcx>) -> ConstOperand<'tcx> {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
- // MIR for `repeat_once_to_aggregate` before InstSimplify-after-simplifycfg
+ // MIR for `repeat_once_to_aggregate` after InstSimplify-after-simplifycfg

fn repeat_once_to_aggregate(_1: T) -> [T; 1] {
debug x => _1;
let mut _0: [T; 1];
let mut _2: T;

bb0: {
StorageLive(_2);
_2 = copy _1;
- _0 = [move _2; 1];
+ _0 = [move _2];
StorageDead(_2);
return;
}
}

13 changes: 13 additions & 0 deletions tests/mir-opt/instsimplify/simplify_repeat.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
//@ test-mir-pass: InstSimplify-after-simplifycfg
//@ compile-flags: -C panic=abort
#![crate_type = "lib"]

// EMIT_MIR simplify_repeat.repeat_once_to_aggregate.InstSimplify-after-simplifycfg.diff
pub fn repeat_once_to_aggregate<T: Copy>(x: T) -> [T; 1] {
// CHECK-LABEL: fn repeat_once_to_aggregate(
// CHECK-NOT: [move {{_[0-9]+}}; 1]
// CHECK: _0 = [move {{_[0-9]+}}];
// CHECK-NOT: [move {{_[0-9]+}}; 1]

[x; 1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Also test getting the 1 from a constant, not a literal. I think the normalization for that has already happened, but we've had some issues with that recently

}
10 changes: 0 additions & 10 deletions tests/mir-opt/remove_zsts.get_union.PreCodegen.after.mir

This file was deleted.

29 changes: 29 additions & 0 deletions tests/mir-opt/remove_zsts.remove_generic_array.RemoveZsts.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
- // MIR for `remove_generic_array` before RemoveZsts
+ // MIR for `remove_generic_array` after RemoveZsts

fn remove_generic_array(_1: T) -> () {
debug x => _1;
let mut _0: ();
let _2: [T; 0];
let mut _3: T;
scope 1 {
- debug a => _2;
+ debug a => const ZeroSized: [T; 0];
}

bb0: {
- StorageLive(_2);
+ nop;
StorageLive(_3);
_3 = copy _1;
- _2 = [];
+ nop;
StorageDead(_3);
- _0 = const ();
- StorageDead(_2);
+ nop;
+ nop;
return;
}
}

14 changes: 12 additions & 2 deletions tests/mir-opt/remove_zsts.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,25 @@
// skip-filecheck
//@ test-mir-pass: RemoveZsts

union Foo {
x: (),
y: u64,
}

// EMIT_MIR remove_zsts.get_union.RemoveZsts.diff
// EMIT_MIR remove_zsts.get_union.PreCodegen.after.mir
fn get_union() -> Foo {
// CHECK-LABEL: fn get_union
// CHECK: _0 = Foo { x: const () };
Foo { x: () }
}

// EMIT_MIR remove_zsts.remove_generic_array.RemoveZsts.diff
fn remove_generic_array<T: Copy>(x: T) {
// CHECK-LABEL: fn remove_generic_array
// CHECK: debug a => const ZeroSized: [T; 0];
// CHECK-NOT: = [];
let a = [x; 0];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also add a test where the zero comes from a constant not a literal


fn main() {
get_union();
}
Loading