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

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.

11 files changed, 52 insertions, 87 deletions

diff --git a/compiler/rustc_codegen_llvm/src/intrinsic.rs b/compiler/rustc_codegen_llvm/src/intrinsic.rs
index 668daa52ed2..f445d708c94 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 e148ed7ad3b..a8dda100763 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<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);
             }
 
diff --git a/compiler/rustc_codegen_ssa/src/mir/mod.rs b/compiler/rustc_codegen_ssa/src/mir/mod.rs
index d31ececf130..3f945478213 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 ac3c99f9c90..777436ad2ae 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 28646973620..93ba2e6a4f1 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 79bbad8307b..d9e5a186073 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 e54298eed05..00000000000
--- 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 56b360e0a7f..8f9a53d19d4 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 2921ab6dc04..a4c76de1e3b 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 1f292ce379f..ccb22afbc7a 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 00000000000..896b7e8721c
--- /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
+}