From 31ee8b18188c748bf73cce52478c61629ed889cc Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Thu, 10 Mar 2022 17:10:36 -0500 Subject: [PATCH 1/4] Reapply: Mark drop calls in landing pads cold instead of noinline Co-authored-by: Max Fan Co-authored-by: Nikita Popov --- compiler/rustc_codegen_gcc/src/builder.rs | 2 +- compiler/rustc_codegen_llvm/src/builder.rs | 8 ++-- compiler/rustc_codegen_ssa/src/mir/block.rs | 10 ++--- .../rustc_codegen_ssa/src/traits/builder.rs | 2 +- tests/codegen/issue-97217.rs | 21 ++++++++++ tests/codegen/unwind-landingpad-cold.rs | 14 +++++++ tests/codegen/unwind-landingpad-inline.rs | 40 +++++++++++++++++++ 7 files changed, 85 insertions(+), 12 deletions(-) create mode 100644 tests/codegen/issue-97217.rs create mode 100644 tests/codegen/unwind-landingpad-cold.rs create mode 100644 tests/codegen/unwind-landingpad-inline.rs diff --git a/compiler/rustc_codegen_gcc/src/builder.rs b/compiler/rustc_codegen_gcc/src/builder.rs index 308cb04cac3d5..ecc293aee237b 100644 --- a/compiler/rustc_codegen_gcc/src/builder.rs +++ b/compiler/rustc_codegen_gcc/src/builder.rs @@ -1420,7 +1420,7 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { self.cx } - fn do_not_inline(&mut self, _llret: RValue<'gcc>) { + fn apply_attrs_to_cleanup_callsite(&mut self, _llret: RValue<'gcc>) { // FIXME(bjorn3): implement } diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index ac6d8f84142e7..057e275945498 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -1225,9 +1225,11 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { unsafe { llvm::LLVMBuildZExt(self.llbuilder, val, dest_ty, UNNAMED) } } - fn do_not_inline(&mut self, llret: &'ll Value) { - let noinline = llvm::AttributeKind::NoInline.create_attr(self.llcx); - attributes::apply_to_callsite(llret, llvm::AttributePlace::Function, &[noinline]); + fn apply_attrs_to_cleanup_callsite(&mut self, llret: &'ll Value) { + // Cleanup is always the cold path. + let cold_inline = llvm::AttributeKind::Cold.create_attr(self.llcx); + + attributes::apply_to_callsite(llret, llvm::AttributePlace::Function, &[cold_inline]); } } diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index bd0707edfd99d..a0cb97d51a01f 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -213,7 +213,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> { self.funclet(fx), ); if fx.mir[self.bb].is_cleanup { - bx.do_not_inline(invokeret); + bx.apply_attrs_to_cleanup_callsite(invokeret); } if let Some((ret_dest, target)) = destination { @@ -228,11 +228,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> { } else { let llret = bx.call(fn_ty, fn_attrs, Some(&fn_abi), fn_ptr, &llargs, self.funclet(fx)); if fx.mir[self.bb].is_cleanup { - // Cleanup is always the cold path. Don't inline - // drop glue. Also, when there is a deeply-nested - // struct, there are "symmetry" issues that cause - // exponential inlining - see issue #41696. - bx.do_not_inline(llret); + bx.apply_attrs_to_cleanup_callsite(llret); } if let Some((ret_dest, target)) = destination { @@ -1627,7 +1623,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let fn_ty = bx.fn_decl_backend_type(&fn_abi); let llret = bx.call(fn_ty, None, Some(&fn_abi), fn_ptr, &[], funclet.as_ref()); - bx.do_not_inline(llret); + bx.apply_attrs_to_cleanup_callsite(llret); bx.unreachable(); diff --git a/compiler/rustc_codegen_ssa/src/traits/builder.rs b/compiler/rustc_codegen_ssa/src/traits/builder.rs index 853c6934c2c24..aa411f002a0c6 100644 --- a/compiler/rustc_codegen_ssa/src/traits/builder.rs +++ b/compiler/rustc_codegen_ssa/src/traits/builder.rs @@ -332,5 +332,5 @@ pub trait BuilderMethods<'a, 'tcx>: ) -> Self::Value; fn zext(&mut self, val: Self::Value, dest_ty: Self::Type) -> Self::Value; - fn do_not_inline(&mut self, llret: Self::Value); + fn apply_attrs_to_cleanup_callsite(&mut self, llret: Self::Value); } diff --git a/tests/codegen/issue-97217.rs b/tests/codegen/issue-97217.rs new file mode 100644 index 0000000000000..5558447cd1781 --- /dev/null +++ b/tests/codegen/issue-97217.rs @@ -0,0 +1,21 @@ +// compile-flags: -C opt-level=3 +// ignore-debug: the debug assertions get in the way +#![crate_type = "lib"] + +// Regression test for issue 97217 (the following should result in no allocations) + +// CHECK-LABEL: @issue97217 +#[no_mangle] +pub fn issue97217() -> i32 { + // drop_in_place should be inlined and never appear + // CHECK-NOT: drop_in_place + + // __rust_alloc should be optimized out + // CHECK-NOT: __rust_alloc + + let v1 = vec![5, 6, 7]; + let v1_iter = v1.iter(); + let total: i32 = v1_iter.sum(); + println!("{}",total); + total +} diff --git a/tests/codegen/unwind-landingpad-cold.rs b/tests/codegen/unwind-landingpad-cold.rs new file mode 100644 index 0000000000000..afebc31a23a39 --- /dev/null +++ b/tests/codegen/unwind-landingpad-cold.rs @@ -0,0 +1,14 @@ +// compile-flags: -Cno-prepopulate-passes +#![crate_type = "lib"] + +// This test checks that drop calls in unwind landing pads +// get the `cold` attribute. + +// CHECK-LABEL: @check_cold +// CHECK: {{(call|invoke) void .+}}drop_in_place{{.+}} [[ATTRIBUTES:#[0-9]+]] +// CHECK: attributes [[ATTRIBUTES]] = { cold } +#[no_mangle] +pub fn check_cold(f: fn(), x: Box) { + // this may unwind + f(); +} diff --git a/tests/codegen/unwind-landingpad-inline.rs b/tests/codegen/unwind-landingpad-inline.rs new file mode 100644 index 0000000000000..10fd831a6de0c --- /dev/null +++ b/tests/codegen/unwind-landingpad-inline.rs @@ -0,0 +1,40 @@ +// min-llvm-version: 15.0.0 +// compile-flags: -Copt-level=3 +// ignore-debug: the debug assertions get in the way +#![crate_type = "lib"] + +// This test checks that we can inline drop_in_place in +// unwind landing pads. + +// Without inlining, the box pointers escape via the call to drop_in_place, +// and LLVM will not optimize out the pointer comparison. +// With inlining, everything should be optimized out. +// See https://github.com/rust-lang/rust/issues/46515 +// CHECK-LABEL: @check_no_escape_in_landingpad +// CHECK: start: +// CHECK-NEXT: __rust_no_alloc_shim_is_unstable +// CHECK-NEXT: __rust_no_alloc_shim_is_unstable +// CHECK-NEXT: ret void +#[no_mangle] +pub fn check_no_escape_in_landingpad(f: fn()) { + let x = &*Box::new(0); + let y = &*Box::new(0); + + if x as *const _ == y as *const _ { + f(); + } +} + +// Without inlining, the compiler can't tell that +// dropping an empty string (in a landing pad) does nothing. +// With inlining, the landing pad should be optimized out. +// See https://github.com/rust-lang/rust/issues/87055 +// CHECK-LABEL: @check_eliminate_noop_drop +// CHECK: start: +// CHECK-NEXT: call void %g() +// CHECK-NEXT: ret void +#[no_mangle] +pub fn check_eliminate_noop_drop(g: fn()) { + let _var = String::new(); + g(); +} From 0608fca3adec23ad95e49eec2e17a4377c7c634a Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Tue, 8 Aug 2023 11:28:37 +0200 Subject: [PATCH 2/4] Fix codegen tests on panic=abort targets --- tests/codegen/unwind-landingpad-cold.rs | 1 + tests/codegen/unwind-landingpad-inline.rs | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/codegen/unwind-landingpad-cold.rs b/tests/codegen/unwind-landingpad-cold.rs index afebc31a23a39..53d0a01a9523c 100644 --- a/tests/codegen/unwind-landingpad-cold.rs +++ b/tests/codegen/unwind-landingpad-cold.rs @@ -1,4 +1,5 @@ // compile-flags: -Cno-prepopulate-passes +// needs-unwind #![crate_type = "lib"] // This test checks that drop calls in unwind landing pads diff --git a/tests/codegen/unwind-landingpad-inline.rs b/tests/codegen/unwind-landingpad-inline.rs index 10fd831a6de0c..a65d6f202f6f4 100644 --- a/tests/codegen/unwind-landingpad-inline.rs +++ b/tests/codegen/unwind-landingpad-inline.rs @@ -30,8 +30,7 @@ pub fn check_no_escape_in_landingpad(f: fn()) { // With inlining, the landing pad should be optimized out. // See https://github.com/rust-lang/rust/issues/87055 // CHECK-LABEL: @check_eliminate_noop_drop -// CHECK: start: -// CHECK-NEXT: call void %g() +// CHECK: call void %g() // CHECK-NEXT: ret void #[no_mangle] pub fn check_eliminate_noop_drop(g: fn()) { From ebbc68769d1207e8036cfbff00e5b8016e9a16b7 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Tue, 8 Aug 2023 11:34:07 +0200 Subject: [PATCH 3/4] Update stack protector test We no longer generate a protector for the strong case in this test, which is actually the expected behavior per the test comment. --- .../stack-protector/stack-protector-heuristics-effect.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/assembly/stack-protector/stack-protector-heuristics-effect.rs b/tests/assembly/stack-protector/stack-protector-heuristics-effect.rs index a7c9e4845c70a..91b218c7a3cfb 100644 --- a/tests/assembly/stack-protector/stack-protector-heuristics-effect.rs +++ b/tests/assembly/stack-protector/stack-protector-heuristics-effect.rs @@ -371,7 +371,7 @@ pub fn unsized_fn_param(s: [u8], l: bool, f: fn([u8])) { // all: __stack_chk_fail - // strong: __stack_chk_fail + // strong-NOT: __stack_chk_fail // basic-NOT: __stack_chk_fail // none-NOT: __stack_chk_fail // missing-NOT: __stack_chk_fail From 5bcf4f26ac44284b0dbc9b53404fa5dd8a0db01a Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Mon, 2 Oct 2023 10:45:49 +0200 Subject: [PATCH 4/4] Limit to LLVM 17.0.2 to work around WinEH codegen bug --- compiler/rustc_codegen_llvm/src/builder.rs | 14 ++++++++++---- .../stack-protector-heuristics-effect.rs | 1 + tests/codegen/issue-97217.rs | 1 + tests/codegen/unwind-landingpad-cold.rs | 1 + tests/codegen/unwind-landingpad-inline.rs | 2 +- 5 files changed, 14 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index 057e275945498..4f9b86ec20a7b 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -3,6 +3,7 @@ use crate::attributes; use crate::common::Funclet; use crate::context::CodegenCx; use crate::llvm::{self, AtomicOrdering, AtomicRmwBinOp, BasicBlock, False, True}; +use crate::llvm_util; use crate::type_::Type; use crate::type_of::LayoutLlvmExt; use crate::value::Value; @@ -1226,10 +1227,15 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { } fn apply_attrs_to_cleanup_callsite(&mut self, llret: &'ll Value) { - // Cleanup is always the cold path. - let cold_inline = llvm::AttributeKind::Cold.create_attr(self.llcx); - - attributes::apply_to_callsite(llret, llvm::AttributePlace::Function, &[cold_inline]); + if llvm_util::get_version() < (17, 0, 2) { + // Work around https://github.com/llvm/llvm-project/issues/66984. + let noinline = llvm::AttributeKind::NoInline.create_attr(self.llcx); + attributes::apply_to_callsite(llret, llvm::AttributePlace::Function, &[noinline]); + } else { + // Cleanup is always the cold path. + let cold_inline = llvm::AttributeKind::Cold.create_attr(self.llcx); + attributes::apply_to_callsite(llret, llvm::AttributePlace::Function, &[cold_inline]); + } } } diff --git a/tests/assembly/stack-protector/stack-protector-heuristics-effect.rs b/tests/assembly/stack-protector/stack-protector-heuristics-effect.rs index 91b218c7a3cfb..e46b902df0795 100644 --- a/tests/assembly/stack-protector/stack-protector-heuristics-effect.rs +++ b/tests/assembly/stack-protector/stack-protector-heuristics-effect.rs @@ -9,6 +9,7 @@ // [basic] compile-flags: -Z stack-protector=basic // [none] compile-flags: -Z stack-protector=none // compile-flags: -C opt-level=2 -Z merge-functions=disabled +// min-llvm-version: 17.0.2 #![crate_type = "lib"] diff --git a/tests/codegen/issue-97217.rs b/tests/codegen/issue-97217.rs index 5558447cd1781..af7345442fc51 100644 --- a/tests/codegen/issue-97217.rs +++ b/tests/codegen/issue-97217.rs @@ -1,5 +1,6 @@ // compile-flags: -C opt-level=3 // ignore-debug: the debug assertions get in the way +// min-llvm-version: 17.0.2 #![crate_type = "lib"] // Regression test for issue 97217 (the following should result in no allocations) diff --git a/tests/codegen/unwind-landingpad-cold.rs b/tests/codegen/unwind-landingpad-cold.rs index 53d0a01a9523c..3a902a7d71238 100644 --- a/tests/codegen/unwind-landingpad-cold.rs +++ b/tests/codegen/unwind-landingpad-cold.rs @@ -1,5 +1,6 @@ // compile-flags: -Cno-prepopulate-passes // needs-unwind +// min-llvm-version: 17.0.2 #![crate_type = "lib"] // This test checks that drop calls in unwind landing pads diff --git a/tests/codegen/unwind-landingpad-inline.rs b/tests/codegen/unwind-landingpad-inline.rs index a65d6f202f6f4..0774cefdd2d1a 100644 --- a/tests/codegen/unwind-landingpad-inline.rs +++ b/tests/codegen/unwind-landingpad-inline.rs @@ -1,4 +1,4 @@ -// min-llvm-version: 15.0.0 +// min-llvm-version: 17.0.2 // compile-flags: -Copt-level=3 // ignore-debug: the debug assertions get in the way #![crate_type = "lib"]