bjoernager/rust
bjoernager/rust
1
Fork 0
Code Issues Pull requests Activity

Auto merge of #82884 - nagisa:nagisa/remove-most-of-sideeffect-inserts, r=nikic

Browse source 
Remove the -Zinsert-sideeffect

This removes all of the code we had in place to work-around LLVM's
handling of forward progress. From this removal excluded is a workaround
where we'd insert a `sideeffect` into clearly infinite loops such as
`loop {}`. This code remains conditionally effective when the LLVM
version is earlier than 12.0, which fixed the forward progress related
miscompilations at their root.
This commit is contained in:
bors 2021-03-10 14:03:00 +00:00
commit 5fe790e3c4
11 changed files with 52 additions and 87 deletions
Download patch file Download diff file Expand all files Collapse all files

58
compiler/rustc_codegen_ssa/src/mir/block.rs
View file

@ -146,24 +146,6 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
}
}
}
// Generate sideeffect intrinsic if jumping to any of the targets can form
// a loop.
fn maybe_sideeffect<Bx: BuilderMethods<'a, 'tcx>>(
&self,
mir: &'tcx mir::Body<'tcx>,
bx: &mut Bx,
targets: &[mir::BasicBlock],
) {
if bx.tcx().sess.opts.debugging_opts.insert_sideeffect {
if targets.iter().any(|&target| {
target <= self.bb
&& target.start_location().is_predecessor_of(self.bb.start_location(), mir)
}) {
bx.sideeffect(false);
}
}
}
}
/// Codegen implementations for some terminator variants.
@ -198,8 +180,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let discr = self.codegen_operand(&mut bx, &discr);
// `switch_ty` is redundant, sanity-check that.
assert_eq!(discr.layout.ty, switch_ty);
helper.maybe_sideeffect(self.mir, &mut bx, targets.all_targets());
let mut target_iter = targets.iter();
if target_iter.len() == 1 {
// If there are two targets (one conditional, one fallback), emit br instead of switch
@ -308,7 +288,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
if let ty::InstanceDef::DropGlue(_, None) = drop_fn.def {
// we don't actually need to drop anything.
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
helper.funclet_br(self, &mut bx, target);
return;
}
@ -337,7 +316,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}
_ => (bx.get_fn_addr(drop_fn), FnAbi::of_instance(&bx, drop_fn, &[])),
};
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
helper.do_call(
self,
&mut bx,
@ -379,7 +357,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
// Don't codegen the panic block if success if known.
if const_cond == Some(expected) {
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
helper.funclet_br(self, &mut bx, target);
return;
}
@ -390,7 +367,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
// Create the failure block and the conditional branch to it.
let lltarget = helper.llblock(self, target);
let panic_block = self.new_block("panic");
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
if expected {
bx.cond_br(cond, lltarget, panic_block.llbb());
} else {
@ -491,9 +467,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let fn_abi = FnAbi::of_instance(bx, instance, &[]);
let llfn = bx.get_fn_addr(instance);
if let Some((_, target)) = destination.as_ref() {
helper.maybe_sideeffect(self.mir, bx, &[*target]);
}
// Codegen the actual panic invoke/call.
helper.do_call(
self,
@ -507,7 +480,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
} else {
// a NOP
let target = destination.as_ref().unwrap().1;
helper.maybe_sideeffect(self.mir, bx, &[target]);
helper.funclet_br(self, bx, target)
}
true
@ -551,7 +523,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
if let Some(ty::InstanceDef::DropGlue(_, None)) = def {
// Empty drop glue; a no-op.
let &(_, target) = destination.as_ref().unwrap();
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
helper.funclet_br(self, &mut bx, target);
return;
}
@ -586,7 +557,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
if let Some(destination_ref) = destination.as_ref() {
let &(dest, target) = destination_ref;
self.codegen_transmute(&mut bx, &args[0], dest);
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
helper.funclet_br(self, &mut bx, target);
} else {
// If we are trying to transmute to an uninhabited type,
@ -634,8 +604,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
location.val.store(&mut bx, tmp);
}
self.store_return(&mut bx, ret_dest, &fn_abi.ret, location.immediate());
helper.maybe_sideeffect(self.mir, &mut bx, &[*target]);
helper.funclet_br(self, &mut bx, *target);
}
return;
@ -700,7 +668,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}
if let Some((_, target)) = *destination {
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
helper.funclet_br(self, &mut bx, target);
} else {
bx.unreachable();
@ -817,9 +784,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
_ => span_bug!(span, "no llfn for call"),
};
if let Some((_, target)) = destination.as_ref() {
helper.maybe_sideeffect(self.mir, &mut bx, &[*target]);
}
helper.do_call(
self,
&mut bx,
@ -969,22 +933,16 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
mir::TerminatorKind::Goto { target } => {
if bb == target {
// This is an unconditional branch back to this same basic
// block. That means we have something like a `loop {}`
// statement. Currently LLVM miscompiles this because it
// assumes forward progress. We want to prevent this in all
// cases, but that has a fairly high cost to compile times
// currently. Instead, try to handle this specific case
// which comes up commonly in practice (e.g., in embedded
// code).
// This is an unconditional branch back to this same basic block. That means we
// have something like a `loop {}` statement. LLVM versions before 12.0
// miscompile this because they assume forward progress. For older versions
// try to handle just this specific case which comes up commonly in practice
// (e.g., in embedded code).
//
// The `true` here means we insert side effects regardless
// of -Zinsert-sideeffect being passed on unconditional
// branching to the same basic block.
bx.sideeffect(true);
} else {
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
// NB: the `sideeffect` currently checks for the LLVM version used internally.
bx.sideeffect();
}
helper.funclet_br(self, &mut bx, target);
}

2
compiler/rustc_codegen_ssa/src/mir/mod.rs
View file

@ -149,8 +149,6 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
bx.set_personality_fn(cx.eh_personality());
}
bx.sideeffect(false);
let cleanup_kinds = analyze::cleanup_kinds(&mir);
// Allocate a `Block` for every basic block, except
// the start block, if nothing loops back to it.

7
compiler/rustc_codegen_ssa/src/traits/intrinsic.rs
View file

@ -20,9 +20,10 @@ pub trait IntrinsicCallMethods<'tcx>: BackendTypes {
fn abort(&mut self);
fn assume(&mut self, val: Self::Value);
fn expect(&mut self, cond: Self::Value, expected: bool) -> Self::Value;
/// Normally, sideeffect is only emitted if -Zinsert-sideeffect is passed;
/// in some cases though we want to emit it regardless.
fn sideeffect(&mut self, unconditional: bool);
/// Emits a forced side effect.
///
/// Currently has any effect only when LLVM versions prior to 12.0 are used as the backend.
fn sideeffect(&mut self);
/// Trait method used to inject `va_start` on the "spoofed" `VaListImpl` in
/// Rust defined C-variadic functions.
fn va_start(&mut self, val: Self::Value) -> Self::Value;