diff --git a/compiler/rustc_codegen_llvm/src/intrinsic.rs b/compiler/rustc_codegen_llvm/src/intrinsic.rs index 668daa52ed262..f445d708c94f6 100644 --- a/compiler/rustc_codegen_llvm/src/intrinsic.rs +++ b/compiler/rustc_codegen_llvm/src/intrinsic.rs @@ -334,8 +334,11 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { self.call(expect, &[cond, self.const_bool(expected)], None) } - fn sideeffect(&mut self, unconditional: bool) { - if unconditional || self.tcx.sess.opts.debugging_opts.insert_sideeffect { + fn sideeffect(&mut self) { + // This kind of check would make a ton of sense in the caller, but currently the only + // caller of this function is in `rustc_codegen_ssa`, which is agnostic to whether LLVM + // codegen backend being used, and so is unable to check the LLVM version. + if unsafe { llvm::LLVMRustVersionMajor() } < 12 { let fnname = self.get_intrinsic(&("llvm.sideeffect")); self.call(fnname, &[], None); } @@ -390,7 +393,6 @@ fn codegen_msvc_try( ) { let llfn = get_rust_try_fn(bx, &mut |mut bx| { bx.set_personality_fn(bx.eh_personality()); - bx.sideeffect(false); let mut normal = bx.build_sibling_block("normal"); let mut catchswitch = bx.build_sibling_block("catchswitch"); @@ -552,9 +554,6 @@ fn codegen_gnu_try( // (%ptr, _) = landingpad // call %catch_func(%data, %ptr) // ret 1 - - bx.sideeffect(false); - let mut then = bx.build_sibling_block("then"); let mut catch = bx.build_sibling_block("catch"); @@ -614,9 +613,6 @@ fn codegen_emcc_try( // %catch_data[1] = %is_rust_panic // call %catch_func(%data, %catch_data) // ret 1 - - bx.sideeffect(false); - let mut then = bx.build_sibling_block("then"); let mut catch = bx.build_sibling_block("catch"); diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index e148ed7ad3bce..a8dda100763aa 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -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>( - &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); } diff --git a/compiler/rustc_codegen_ssa/src/mir/mod.rs b/compiler/rustc_codegen_ssa/src/mir/mod.rs index d31ececf13062..3f94547821349 100644 --- a/compiler/rustc_codegen_ssa/src/mir/mod.rs +++ b/compiler/rustc_codegen_ssa/src/mir/mod.rs @@ -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. diff --git a/compiler/rustc_codegen_ssa/src/traits/intrinsic.rs b/compiler/rustc_codegen_ssa/src/traits/intrinsic.rs index ac3c99f9c908d..777436ad2ae8f 100644 --- a/compiler/rustc_codegen_ssa/src/traits/intrinsic.rs +++ b/compiler/rustc_codegen_ssa/src/traits/intrinsic.rs @@ -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; diff --git a/compiler/rustc_interface/src/tests.rs b/compiler/rustc_interface/src/tests.rs index 2864697362057..93ba2e6a4f1e9 100644 --- a/compiler/rustc_interface/src/tests.rs +++ b/compiler/rustc_interface/src/tests.rs @@ -560,7 +560,6 @@ fn test_debugging_options_tracking_hash() { tracked!(inline_mir, Some(true)); tracked!(inline_mir_threshold, Some(123)); tracked!(inline_mir_hint_threshold, Some(123)); - tracked!(insert_sideeffect, true); tracked!(instrument_coverage, true); tracked!(instrument_mcount, true); tracked!(link_only, true); diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs index 79bbad8307ba7..d9e5a186073b6 100644 --- a/compiler/rustc_session/src/options.rs +++ b/compiler/rustc_session/src/options.rs @@ -967,10 +967,6 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "control whether `#[inline]` functions are in all CGUs"), input_stats: bool = (false, parse_bool, [UNTRACKED], "gather statistics about the input (default: no)"), - insert_sideeffect: bool = (false, parse_bool, [TRACKED], - "fix undefined behavior when a thread doesn't eventually make progress \ - (such as entering an empty infinite loop) by inserting llvm.sideeffect \ - (default: no)"), instrument_coverage: bool = (false, parse_bool, [TRACKED], "instrument the generated code to support LLVM source-based code coverage \ reports (note, the compiler build config must include `profiler = true`, \ diff --git a/src/test/codegen/loop.rs b/src/test/codegen/loop.rs deleted file mode 100644 index e54298eed059e..0000000000000 --- a/src/test/codegen/loop.rs +++ /dev/null @@ -1,15 +0,0 @@ -// compile-flags: -C opt-level=3 - -#![crate_type = "lib"] - -// CHECK-LABEL: @check_loop -#[no_mangle] -pub fn check_loop() -> u8 { - // CHECK-NOT: unreachable - call_looper() -} - -#[no_mangle] -fn call_looper() -> ! { - loop {} -} diff --git a/src/test/codegen/non-terminate/infinite-loop-1.rs b/src/test/codegen/non-terminate/infinite-loop-1.rs index 56b360e0a7f48..8f9a53d19d43a 100644 --- a/src/test/codegen/non-terminate/infinite-loop-1.rs +++ b/src/test/codegen/non-terminate/infinite-loop-1.rs @@ -1,4 +1,5 @@ -// compile-flags: -C opt-level=3 -Z insert-sideeffect +// min-llvm-version: 12.0 +// compile-flags: -C opt-level=3 #![crate_type = "lib"] diff --git a/src/test/codegen/non-terminate/infinite-loop-2.rs b/src/test/codegen/non-terminate/infinite-loop-2.rs index 2921ab6dc04af..a4c76de1e3b8a 100644 --- a/src/test/codegen/non-terminate/infinite-loop-2.rs +++ b/src/test/codegen/non-terminate/infinite-loop-2.rs @@ -1,4 +1,5 @@ -// compile-flags: -C opt-level=3 -Z insert-sideeffect +// min-llvm-version: 12.0 +// compile-flags: -C opt-level=3 #![crate_type = "lib"] diff --git a/src/test/codegen/non-terminate/infinite-recursion.rs b/src/test/codegen/non-terminate/infinite-recursion.rs index 1f292ce379fc0..ccb22afbc7ae0 100644 --- a/src/test/codegen/non-terminate/infinite-recursion.rs +++ b/src/test/codegen/non-terminate/infinite-recursion.rs @@ -1,4 +1,5 @@ -// compile-flags: -C opt-level=3 -Z insert-sideeffect +// min-llvm-version: 12.0 +// compile-flags: -C opt-level=3 #![crate_type = "lib"] diff --git a/src/test/codegen/non-terminate/nonempty-infinite-loop.rs b/src/test/codegen/non-terminate/nonempty-infinite-loop.rs new file mode 100644 index 0000000000000..896b7e8721cb7 --- /dev/null +++ b/src/test/codegen/non-terminate/nonempty-infinite-loop.rs @@ -0,0 +1,29 @@ +// min-llvm-version: 12.0 +// compile-flags: -C opt-level=3 + +#![crate_type = "lib"] + +// Verify that we don't miscompile this even if rustc didn't apply the trivial loop detection to +// insert the sideeffect intrinsic. + +fn infinite_loop() -> u8 { + let mut x = 0; + // CHECK-NOT: sideeffect + loop { + if x == 42 { + x = 0; + } else { + x = 42; + } + } +} + +// CHECK-LABEL: @test +#[no_mangle] +fn test() -> u8 { + // CHECK-NOT: unreachable + // CHECK: br label %{{.+}} + // CHECK-NOT: unreachable + let x = infinite_loop(); + x +}