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

Reapply: Mark drop calls in landing pads cold instead of noinline #94823

Closed
wants to merge 2 commits into from
Closed
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
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_gcc/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1403,7 +1403,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>) {
unimplemented!();
}

Expand Down
17 changes: 14 additions & 3 deletions compiler/rustc_codegen_llvm/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_span::Span;
use rustc_target::abi::{self, call::FnAbi, Align, Size, WrappingRange};
use rustc_target::spec::{HasTargetSpec, Target};
use smallvec::SmallVec;
use std::borrow::Cow;
use std::ffi::CStr;
use std::iter;
Expand Down Expand Up @@ -1188,9 +1189,19 @@ 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) {
let mut attrs = SmallVec::<[_; 2]>::new();

// Cleanup is always the cold path.
attrs.push(llvm::AttributeKind::Cold.create_attr(self.llcx));

// LLVM 14 contains fixes for catastrophic inlining behavior, without which
// inlining drop glue can lead to exponential size blowup, see #41696 and #92110.
if llvm_util::get_version() < (14, 0, 0) {
attrs.push(llvm::AttributeKind::NoInline.create_attr(self.llcx));
}

attributes::apply_to_callsite(llret, llvm::AttributePlace::Function, &attrs);
}
}

Expand Down
10 changes: 3 additions & 7 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
bx.invoke(fn_ty, fn_ptr, &llargs, ret_llbb, unwind_block, self.funclet(fx));
bx.apply_attrs_callsite(&fn_abi, invokeret);
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 {
Expand All @@ -178,11 +178,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
let llret = bx.call(fn_ty, fn_ptr, &llargs, self.funclet(fx));
bx.apply_attrs_callsite(&fn_abi, llret);
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 {
Expand Down Expand Up @@ -1448,7 +1444,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

let llret = bx.call(fn_ty, fn_ptr, &[], None);
bx.apply_attrs_callsite(&fn_abi, llret);
bx.do_not_inline(llret);
bx.apply_attrs_to_cleanup_callsite(llret);

bx.unreachable();

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/traits/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -479,5 +479,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);
}
15 changes: 15 additions & 0 deletions src/test/codegen/unwind-landingpad-cold.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// no-system-llvm: needs #92110
// 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<u32>) {
// this may unwind
f();
}
37 changes: 37 additions & 0 deletions src/test/codegen/unwind-landingpad-inline.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// no-system-llvm: needs #92110 + patch for Rust alloc/dealloc functions
// compile-flags: -Copt-level=3
#![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: 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();
}
19 changes: 0 additions & 19 deletions src/test/codegen/vec-shrink-panik.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,32 +16,13 @@ pub fn shrink_to_fit(vec: &mut Vec<u32>) {
// CHECK-LABEL: @issue71861
#[no_mangle]
pub fn issue71861(vec: Vec<u32>) -> Box<[u32]> {
// CHECK-NOT: panic

// Call to panic_no_unwind in case of double-panic is expected,
// but other panics are not.
// CHECK: cleanup
// CHECK-NEXT: ; call core::panicking::panic_no_unwind
// CHECK-NEXT: panic_no_unwind

// CHECK-NOT: panic
vec.into_boxed_slice()
}

// CHECK-LABEL: @issue75636
#[no_mangle]
pub fn issue75636<'a>(iter: &[&'a str]) -> Box<[&'a str]> {
// CHECK-NOT: panic

// Call to panic_no_unwind in case of double-panic is expected,
// but other panics are not.
// CHECK: cleanup
// CHECK-NEXT: ; call core::panicking::panic_no_unwind
// CHECK-NEXT: panic_no_unwind

// CHECK-NOT: panic
iter.iter().copied().collect()
}

// CHECK: ; core::panicking::panic_no_unwind
// CHECK: declare void @{{.*}}panic_no_unwind