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

InstCombine away intrinsic validity assertions #105582

Merged
merged 2 commits into from
Jan 26, 2023
Merged
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
7 changes: 5 additions & 2 deletions compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ mod simd;
pub(crate) use cpuid::codegen_cpuid_call;
pub(crate) use llvm::codegen_llvm_intrinsic_call;

use rustc_middle::ty::layout::HasParamEnv;
use rustc_middle::ty::print::with_no_trimmed_paths;
use rustc_middle::ty::subst::SubstsRef;
use rustc_span::symbol::{kw, sym, Symbol};
Expand Down Expand Up @@ -659,7 +660,9 @@ fn codegen_regular_intrinsic_call<'tcx>(
return;
}

if intrinsic == sym::assert_zero_valid && !fx.tcx.permits_zero_init(layout) {
if intrinsic == sym::assert_zero_valid
&& !fx.tcx.permits_zero_init(fx.param_env().and(layout))
{
with_no_trimmed_paths!({
crate::base::codegen_panic(
fx,
Expand All @@ -674,7 +677,7 @@ fn codegen_regular_intrinsic_call<'tcx>(
}

if intrinsic == sym::assert_mem_uninitialized_valid
&& !fx.tcx.permits_uninit_init(layout)
&& !fx.tcx.permits_uninit_init(fx.param_env().and(layout))
{
with_no_trimmed_paths!({
crate::base::codegen_panic(
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -678,8 +678,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let layout = bx.layout_of(ty);
let do_panic = match intrinsic {
Inhabited => layout.abi.is_uninhabited(),
ZeroValid => !bx.tcx().permits_zero_init(layout),
MemUninitializedValid => !bx.tcx().permits_uninit_init(layout),
ZeroValid => !bx.tcx().permits_zero_init(bx.param_env().and(layout)),
MemUninitializedValid => !bx.tcx().permits_uninit_init(bx.param_env().and(layout)),
};
Some(if do_panic {
let msg_str = with_no_visible_paths!({
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_const_eval/src/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}

if intrinsic_name == sym::assert_zero_valid {
let should_panic = !self.tcx.permits_zero_init(layout);
let should_panic = !self.tcx.permits_zero_init(self.param_env.and(layout));

if should_panic {
M::abort(
Expand All @@ -463,7 +463,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}

if intrinsic_name == sym::assert_mem_uninitialized_valid {
let should_panic = !self.tcx.permits_uninit_init(layout);
let should_panic = !self.tcx.permits_uninit_init(self.param_env.and(layout));

if should_panic {
M::abort(
Expand Down
11 changes: 8 additions & 3 deletions compiler/rustc_const_eval/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,12 @@ pub fn provide(providers: &mut Providers) {
let (param_env, value) = param_env_and_value.into_parts();
const_eval::deref_mir_constant(tcx, param_env, value)
};
providers.permits_uninit_init =
|tcx, ty| util::might_permit_raw_init(tcx, ty, InitKind::UninitMitigated0x01Fill);
providers.permits_zero_init = |tcx, ty| util::might_permit_raw_init(tcx, ty, InitKind::Zero);
providers.permits_uninit_init = |tcx, param_env_and_ty| {
let (param_env, ty) = param_env_and_ty.into_parts();
util::might_permit_raw_init(tcx, param_env, ty, InitKind::UninitMitigated0x01Fill)
};
providers.permits_zero_init = |tcx, param_env_and_ty| {
let (param_env, ty) = param_env_and_ty.into_parts();
util::might_permit_raw_init(tcx, param_env, ty, InitKind::Zero)
};
}
3 changes: 2 additions & 1 deletion compiler/rustc_const_eval/src/util/might_permit_raw_init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,14 @@ use crate::interpret::{InterpCx, MemoryKind, OpTy};
/// to the full uninit check).
pub fn might_permit_raw_init<'tcx>(
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
ty: TyAndLayout<'tcx>,
kind: InitKind,
) -> bool {
if tcx.sess.opts.unstable_opts.strict_init_checks {
might_permit_raw_init_strict(ty, tcx, kind)
} else {
let layout_cx = LayoutCx { tcx, param_env: ParamEnv::reveal_all() };
let layout_cx = LayoutCx { tcx, param_env };
might_permit_raw_init_lax(ty, &layout_cx, kind)
}
}
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2109,12 +2109,12 @@ rustc_queries! {
separate_provide_extern
}

query permits_uninit_init(key: TyAndLayout<'tcx>) -> bool {
desc { "checking to see if `{}` permits being left uninit", key.ty }
query permits_uninit_init(key: ty::ParamEnvAnd<'tcx, TyAndLayout<'tcx>>) -> bool {
desc { "checking to see if `{}` permits being left uninit", key.value.ty }
}

query permits_zero_init(key: TyAndLayout<'tcx>) -> bool {
desc { "checking to see if `{}` permits being left zeroed", key.ty }
query permits_zero_init(key: ty::ParamEnvAnd<'tcx, TyAndLayout<'tcx>>) -> bool {
desc { "checking to see if `{}` permits being left zeroed", key.value.ty }
}

query compare_impl_const(
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_middle/src/ty/structural_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use crate::ty::{self, InferConst, Lift, Term, TermKind, Ty, TyCtxt};
use rustc_data_structures::functor::IdFunctor;
use rustc_hir::def::Namespace;
use rustc_index::vec::{Idx, IndexVec};
use rustc_target::abi::TyAndLayout;

use std::fmt;
use std::mem::ManuallyDrop;
Expand Down Expand Up @@ -843,3 +844,9 @@ impl<'tcx> TypeSuperVisitable<'tcx> for ty::UnevaluatedConst<'tcx> {
self.substs.visit_with(visitor)
}
}

impl<'tcx> TypeVisitable<'tcx> for TyAndLayout<'tcx, Ty<'tcx>> {
fn visit_with<V: TypeVisitor<'tcx>>(&self, visitor: &mut V) -> ControlFlow<V::BreakTy> {
visitor.visit_ty(self.ty)
}
}
86 changes: 84 additions & 2 deletions compiler/rustc_mir_transform/src/instcombine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ use rustc_middle::mir::{
BinOp, Body, Constant, ConstantKind, LocalDecls, Operand, Place, ProjectionElem, Rvalue,
SourceInfo, Statement, StatementKind, Terminator, TerminatorKind, UnOp,
};
use rustc_middle::ty::{self, TyCtxt};
use rustc_middle::ty::{self, layout::TyAndLayout, ParamEnv, ParamEnvAnd, SubstsRef, Ty, TyCtxt};
use rustc_span::symbol::{sym, Symbol};

pub struct InstCombine;

Expand All @@ -16,7 +17,11 @@ impl<'tcx> MirPass<'tcx> for InstCombine {
}

fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
let ctx = InstCombineContext { tcx, local_decls: &body.local_decls };
let ctx = InstCombineContext {
tcx,
local_decls: &body.local_decls,
param_env: tcx.param_env_reveal_all_normalized(body.source.def_id()),
};
for block in body.basic_blocks.as_mut() {
for statement in block.statements.iter_mut() {
match statement.kind {
Expand All @@ -33,13 +38,18 @@ impl<'tcx> MirPass<'tcx> for InstCombine {
&mut block.terminator.as_mut().unwrap(),
&mut block.statements,
);
ctx.combine_intrinsic_assert(
&mut block.terminator.as_mut().unwrap(),
&mut block.statements,
);
}
}
}

struct InstCombineContext<'tcx, 'a> {
tcx: TyCtxt<'tcx>,
local_decls: &'a LocalDecls<'tcx>,
param_env: ParamEnv<'tcx>,
}

impl<'tcx> InstCombineContext<'tcx, '_> {
Expand Down Expand Up @@ -200,4 +210,76 @@ impl<'tcx> InstCombineContext<'tcx, '_> {
});
terminator.kind = TerminatorKind::Goto { target: destination_block };
}

fn combine_intrinsic_assert(
&self,
terminator: &mut Terminator<'tcx>,
_statements: &mut Vec<Statement<'tcx>>,
) {
let TerminatorKind::Call { func, target, .. } = &mut terminator.kind else { return; };
let Some(target_block) = target else { return; };
let func_ty = func.ty(self.local_decls, self.tcx);
let Some((intrinsic_name, substs)) = resolve_rust_intrinsic(self.tcx, func_ty) else {
return;
};
// The intrinsics we are interested in have one generic parameter
if substs.is_empty() {
return;
}
let ty = substs.type_at(0);

// Check this is a foldable intrinsic before we query the layout of our generic parameter
let Some(assert_panics) = intrinsic_assert_panics(intrinsic_name) else { return; };
let Ok(layout) = self.tcx.layout_of(self.param_env.and(ty)) else { return; };
if assert_panics(self.tcx, self.param_env.and(layout)) {
// If we know the assert panics, indicate to later opts that the call diverges
*target = None;
} else {
// If we know the assert does not panic, turn the call into a Goto
terminator.kind = TerminatorKind::Goto { target: *target_block };
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I entirely understand the point of this. The intrinsic will anyway compile to a goto in this case. It's not like this ever becomes a runtime check. So what is the win here?

Copy link
Member

Choose a reason for hiding this comment

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

After SimplifyCfg the goto will be gone, shrinking the mir slightly. Also having to evaluate the intrinsic every time you monomorphize is slightly more expensive than evaluating it once at mir optimization time.

}
}
}

fn intrinsic_assert_panics<'tcx>(
intrinsic_name: Symbol,
) -> Option<fn(TyCtxt<'tcx>, ParamEnvAnd<'tcx, TyAndLayout<'tcx>>) -> bool> {
fn inhabited_predicate<'tcx>(
_tcx: TyCtxt<'tcx>,
param_env_and_layout: ParamEnvAnd<'tcx, TyAndLayout<'tcx>>,
) -> bool {
let (_param_env, layout) = param_env_and_layout.into_parts();
layout.abi.is_uninhabited()
}
fn zero_valid_predicate<'tcx>(
tcx: TyCtxt<'tcx>,
param_env_and_layout: ParamEnvAnd<'tcx, TyAndLayout<'tcx>>,
) -> bool {
!tcx.permits_zero_init(param_env_and_layout)
}
fn mem_uninitialized_valid_predicate<'tcx>(
tcx: TyCtxt<'tcx>,
param_env_and_layout: ParamEnvAnd<'tcx, TyAndLayout<'tcx>>,
) -> bool {
!tcx.permits_uninit_init(param_env_and_layout)
}

match intrinsic_name {
sym::assert_inhabited => Some(inhabited_predicate),
sym::assert_zero_valid => Some(zero_valid_predicate),
sym::assert_mem_uninitialized_valid => Some(mem_uninitialized_valid_predicate),
_ => None,
}
}

fn resolve_rust_intrinsic<'tcx>(
tcx: TyCtxt<'tcx>,
func_ty: Ty<'tcx>,
) -> Option<(Symbol, SubstsRef<'tcx>)> {
if let ty::FnDef(def_id, substs) = *func_ty.kind() {
if tcx.is_intrinsic(def_id) {
return Some((tcx.item_name(def_id), substs));
}
}
None
}
42 changes: 42 additions & 0 deletions tests/mir-opt/intrinsic_asserts.generic.InstCombine.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
- // MIR for `generic` before InstCombine
+ // MIR for `generic` after InstCombine

fn generic() -> () {
let mut _0: (); // return place in scope 0 at $DIR/intrinsic_asserts.rs:+0:21: +0:21
let _1: (); // in scope 0 at $DIR/intrinsic_asserts.rs:+1:5: +1:46
let _2: (); // in scope 0 at $DIR/intrinsic_asserts.rs:+2:5: +2:47
let _3: (); // in scope 0 at $DIR/intrinsic_asserts.rs:+3:5: +3:60

bb0: {
StorageLive(_1); // scope 0 at $DIR/intrinsic_asserts.rs:+1:5: +1:46
_1 = assert_inhabited::<T>() -> bb1; // scope 0 at $DIR/intrinsic_asserts.rs:+1:5: +1:46
// mir::Constant
// + span: $DIR/intrinsic_asserts.rs:25:5: 25:44
// + literal: Const { ty: extern "rust-intrinsic" fn() {assert_inhabited::<T>}, val: Value(<ZST>) }
}

bb1: {
StorageDead(_1); // scope 0 at $DIR/intrinsic_asserts.rs:+1:46: +1:47
StorageLive(_2); // scope 0 at $DIR/intrinsic_asserts.rs:+2:5: +2:47
_2 = assert_zero_valid::<T>() -> bb2; // scope 0 at $DIR/intrinsic_asserts.rs:+2:5: +2:47
// mir::Constant
// + span: $DIR/intrinsic_asserts.rs:26:5: 26:45
// + literal: Const { ty: extern "rust-intrinsic" fn() {assert_zero_valid::<T>}, val: Value(<ZST>) }
}

bb2: {
StorageDead(_2); // scope 0 at $DIR/intrinsic_asserts.rs:+2:47: +2:48
StorageLive(_3); // scope 0 at $DIR/intrinsic_asserts.rs:+3:5: +3:60
_3 = assert_mem_uninitialized_valid::<T>() -> bb3; // scope 0 at $DIR/intrinsic_asserts.rs:+3:5: +3:60
// mir::Constant
// + span: $DIR/intrinsic_asserts.rs:27:5: 27:58
// + literal: Const { ty: extern "rust-intrinsic" fn() {assert_mem_uninitialized_valid::<T>}, val: Value(<ZST>) }
}

bb3: {
StorageDead(_3); // scope 0 at $DIR/intrinsic_asserts.rs:+3:60: +3:61
nop; // scope 0 at $DIR/intrinsic_asserts.rs:+0:21: +4:2
return; // scope 0 at $DIR/intrinsic_asserts.rs:+4:2: +4:2
}
}

47 changes: 47 additions & 0 deletions tests/mir-opt/intrinsic_asserts.panics.InstCombine.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
- // MIR for `panics` before InstCombine
+ // MIR for `panics` after InstCombine

fn panics() -> () {
let mut _0: (); // return place in scope 0 at $DIR/intrinsic_asserts.rs:+0:17: +0:17
let _1: (); // in scope 0 at $DIR/intrinsic_asserts.rs:+1:5: +1:50
let _2: (); // in scope 0 at $DIR/intrinsic_asserts.rs:+2:5: +2:49
let _3: (); // in scope 0 at $DIR/intrinsic_asserts.rs:+3:5: +3:62

bb0: {
StorageLive(_1); // scope 0 at $DIR/intrinsic_asserts.rs:+1:5: +1:50
- _1 = assert_inhabited::<Never>() -> bb1; // scope 0 at $DIR/intrinsic_asserts.rs:+1:5: +1:50
+ _1 = assert_inhabited::<Never>(); // scope 0 at $DIR/intrinsic_asserts.rs:+1:5: +1:50
// mir::Constant
// + span: $DIR/intrinsic_asserts.rs:17:5: 17:48
// + literal: Const { ty: extern "rust-intrinsic" fn() {assert_inhabited::<Never>}, val: Value(<ZST>) }
}

bb1: {
StorageDead(_1); // scope 0 at $DIR/intrinsic_asserts.rs:+1:50: +1:51
StorageLive(_2); // scope 0 at $DIR/intrinsic_asserts.rs:+2:5: +2:49
- _2 = assert_zero_valid::<&u8>() -> bb2; // scope 0 at $DIR/intrinsic_asserts.rs:+2:5: +2:49
+ _2 = assert_zero_valid::<&u8>(); // scope 0 at $DIR/intrinsic_asserts.rs:+2:5: +2:49
// mir::Constant
// + span: $DIR/intrinsic_asserts.rs:18:5: 18:47
// + user_ty: UserType(0)
// + literal: Const { ty: extern "rust-intrinsic" fn() {assert_zero_valid::<&u8>}, val: Value(<ZST>) }
}

bb2: {
StorageDead(_2); // scope 0 at $DIR/intrinsic_asserts.rs:+2:49: +2:50
StorageLive(_3); // scope 0 at $DIR/intrinsic_asserts.rs:+3:5: +3:62
- _3 = assert_mem_uninitialized_valid::<&u8>() -> bb3; // scope 0 at $DIR/intrinsic_asserts.rs:+3:5: +3:62
+ _3 = assert_mem_uninitialized_valid::<&u8>(); // scope 0 at $DIR/intrinsic_asserts.rs:+3:5: +3:62
// mir::Constant
// + span: $DIR/intrinsic_asserts.rs:19:5: 19:60
// + user_ty: UserType(1)
// + literal: Const { ty: extern "rust-intrinsic" fn() {assert_mem_uninitialized_valid::<&u8>}, val: Value(<ZST>) }
}

bb3: {
StorageDead(_3); // scope 0 at $DIR/intrinsic_asserts.rs:+3:62: +3:63
nop; // scope 0 at $DIR/intrinsic_asserts.rs:+0:17: +4:2
return; // scope 0 at $DIR/intrinsic_asserts.rs:+4:2: +4:2
}
}

45 changes: 45 additions & 0 deletions tests/mir-opt/intrinsic_asserts.removable.InstCombine.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
- // MIR for `removable` before InstCombine
+ // MIR for `removable` after InstCombine

fn removable() -> () {
let mut _0: (); // return place in scope 0 at $DIR/intrinsic_asserts.rs:+0:20: +0:20
let _1: (); // in scope 0 at $DIR/intrinsic_asserts.rs:+1:5: +1:47
let _2: (); // in scope 0 at $DIR/intrinsic_asserts.rs:+2:5: +2:48
let _3: (); // in scope 0 at $DIR/intrinsic_asserts.rs:+3:5: +3:61

bb0: {
StorageLive(_1); // scope 0 at $DIR/intrinsic_asserts.rs:+1:5: +1:47
- _1 = assert_inhabited::<()>() -> bb1; // scope 0 at $DIR/intrinsic_asserts.rs:+1:5: +1:47
- // mir::Constant
- // + span: $DIR/intrinsic_asserts.rs:7:5: 7:45
- // + literal: Const { ty: extern "rust-intrinsic" fn() {assert_inhabited::<()>}, val: Value(<ZST>) }
+ goto -> bb1; // scope 0 at $DIR/intrinsic_asserts.rs:+1:5: +1:47
}

bb1: {
StorageDead(_1); // scope 0 at $DIR/intrinsic_asserts.rs:+1:47: +1:48
StorageLive(_2); // scope 0 at $DIR/intrinsic_asserts.rs:+2:5: +2:48
- _2 = assert_zero_valid::<u8>() -> bb2; // scope 0 at $DIR/intrinsic_asserts.rs:+2:5: +2:48
- // mir::Constant
- // + span: $DIR/intrinsic_asserts.rs:8:5: 8:46
- // + literal: Const { ty: extern "rust-intrinsic" fn() {assert_zero_valid::<u8>}, val: Value(<ZST>) }
+ goto -> bb2; // scope 0 at $DIR/intrinsic_asserts.rs:+2:5: +2:48
}

bb2: {
StorageDead(_2); // scope 0 at $DIR/intrinsic_asserts.rs:+2:48: +2:49
StorageLive(_3); // scope 0 at $DIR/intrinsic_asserts.rs:+3:5: +3:61
- _3 = assert_mem_uninitialized_valid::<u8>() -> bb3; // scope 0 at $DIR/intrinsic_asserts.rs:+3:5: +3:61
- // mir::Constant
- // + span: $DIR/intrinsic_asserts.rs:9:5: 9:59
- // + literal: Const { ty: extern "rust-intrinsic" fn() {assert_mem_uninitialized_valid::<u8>}, val: Value(<ZST>) }
+ goto -> bb3; // scope 0 at $DIR/intrinsic_asserts.rs:+3:5: +3:61
}

bb3: {
StorageDead(_3); // scope 0 at $DIR/intrinsic_asserts.rs:+3:61: +3:62
nop; // scope 0 at $DIR/intrinsic_asserts.rs:+0:20: +4:2
return; // scope 0 at $DIR/intrinsic_asserts.rs:+4:2: +4:2
}
}

Loading