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

Rebased: Mark drop calls in landing pads cold instead of noinline #102099

Merged
merged 4 commits into from
Oct 2, 2023
Merged
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 @@ -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>) {
InnovativeInventor marked this conversation as resolved.
Show resolved Hide resolved
// FIXME(bjorn3): implement
}

Expand Down
14 changes: 11 additions & 3 deletions compiler/rustc_codegen_llvm/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1225,9 +1226,16 @@ 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) {
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]);
}
}
}

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 @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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();

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 @@ -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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -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"]

Expand Down Expand Up @@ -371,7 +372,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
Expand Down
22 changes: 22 additions & 0 deletions tests/codegen/issue-97217.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// 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)

// 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
}
16 changes: 16 additions & 0 deletions tests/codegen/unwind-landingpad-cold.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// 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
// 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();
}
39 changes: 39 additions & 0 deletions tests/codegen/unwind-landingpad-inline.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// min-llvm-version: 17.0.2
// 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: call void %g()
// CHECK-NEXT: ret void
#[no_mangle]
pub fn check_eliminate_noop_drop(g: fn()) {
let _var = String::new();
g();
}
Loading