From 2b662217e745d88d93dcc6f5f5169bf7a0d9720e Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Wed, 29 Dec 2021 15:28:31 -0500 Subject: [PATCH 1/3] Mark drop calls in landing pads cold instead of noinline Now that deferred inlining has been disabled in LLVM, this shouldn't cause catastrophic size blowup. --- compiler/rustc_codegen_gcc/src/builder.rs | 2 +- compiler/rustc_codegen_llvm/src/builder.rs | 4 ++-- compiler/rustc_codegen_ssa/src/mir/block.rs | 7 ++---- .../rustc_codegen_ssa/src/traits/builder.rs | 2 +- src/test/codegen/unwind-landingpad-cold.rs | 14 ++++++++++++ src/test/codegen/unwind-landingpad-inline.rs | 22 +++++++++++++++++++ 6 files changed, 42 insertions(+), 9 deletions(-) create mode 100644 src/test/codegen/unwind-landingpad-cold.rs create mode 100644 src/test/codegen/unwind-landingpad-inline.rs diff --git a/compiler/rustc_codegen_gcc/src/builder.rs b/compiler/rustc_codegen_gcc/src/builder.rs index fff2aa6df7c72..997213d43e8c0 100644 --- a/compiler/rustc_codegen_gcc/src/builder.rs +++ b/compiler/rustc_codegen_gcc/src/builder.rs @@ -1404,7 +1404,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 mark_callsite_cold(&mut self, _llret: RValue<'gcc>) { unimplemented!(); } diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index 8c3054b23ff41..107300ed304f6 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -1201,8 +1201,8 @@ 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) { - llvm::Attribute::NoInline.apply_callsite(llvm::AttributePlace::Function, llret); + fn mark_callsite_cold(&mut self, llret: &'ll Value) { + llvm::Attribute::Cold.apply_callsite(llvm::AttributePlace::Function, llret); } } diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index e914e49326932..8cefd415e28b8 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -160,11 +160,8 @@ 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); + // Cleanup is always the cold path. + bx.mark_callsite_cold(llret); } if let Some((ret_dest, target)) = destination { diff --git a/compiler/rustc_codegen_ssa/src/traits/builder.rs b/compiler/rustc_codegen_ssa/src/traits/builder.rs index 158e658301eed..c1cdff3addadd 100644 --- a/compiler/rustc_codegen_ssa/src/traits/builder.rs +++ b/compiler/rustc_codegen_ssa/src/traits/builder.rs @@ -311,5 +311,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 mark_callsite_cold(&mut self, llret: Self::Value); } diff --git a/src/test/codegen/unwind-landingpad-cold.rs b/src/test/codegen/unwind-landingpad-cold.rs new file mode 100644 index 0000000000000..0bf2941374a66 --- /dev/null +++ b/src/test/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 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/src/test/codegen/unwind-landingpad-inline.rs b/src/test/codegen/unwind-landingpad-inline.rs new file mode 100644 index 0000000000000..d641c908ea99a --- /dev/null +++ b/src/test/codegen/unwind-landingpad-inline.rs @@ -0,0 +1,22 @@ +// no-system-llvm: needs 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 this, the box pointers escape, +// and LLVM will not optimize out the pointer comparison. +// See https://github.com/rust-lang/rust/issues/46515 + +// Everything should be optimized out. +// 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(); + } +} From e4463b2453f3b2665076e5daec23040976b8792e Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Wed, 29 Dec 2021 23:19:55 -0500 Subject: [PATCH 2/3] keep noinline for system llvm < 14 --- compiler/rustc_codegen_gcc/src/builder.rs | 2 +- compiler/rustc_codegen_llvm/src/builder.rs | 9 ++++++++- compiler/rustc_codegen_llvm/src/llvm/ffi.rs | 2 ++ compiler/rustc_codegen_llvm/src/llvm_util.rs | 6 ++++++ compiler/rustc_codegen_ssa/src/mir/block.rs | 3 +-- compiler/rustc_codegen_ssa/src/traits/builder.rs | 2 +- compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp | 8 ++++++++ src/test/codegen/unwind-landingpad-cold.rs | 1 + src/test/codegen/unwind-landingpad-inline.rs | 2 +- 9 files changed, 29 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_codegen_gcc/src/builder.rs b/compiler/rustc_codegen_gcc/src/builder.rs index 997213d43e8c0..cf963531efe2e 100644 --- a/compiler/rustc_codegen_gcc/src/builder.rs +++ b/compiler/rustc_codegen_gcc/src/builder.rs @@ -1404,7 +1404,7 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { self.cx } - fn mark_callsite_cold(&mut self, _llret: RValue<'gcc>) { + fn apply_attrs_to_cleanup_callsite(&mut self, _llret: RValue<'gcc>) { unimplemented!(); } diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index 107300ed304f6..5217fa2758f79 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -1201,8 +1201,15 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { unsafe { llvm::LLVMBuildZExt(self.llbuilder, val, dest_ty, UNNAMED) } } - fn mark_callsite_cold(&mut self, llret: &'ll Value) { + fn apply_attrs_to_cleanup_callsite(&mut self, llret: &'ll Value) { + // Cleanup is always the cold path. llvm::Attribute::Cold.apply_callsite(llvm::AttributePlace::Function, llret); + + // In LLVM versions with deferred inlining (currently, system LLVM < 14), + // inlining drop glue can lead to exponential size blowup, see #41696 and #92110. + if !llvm_util::is_rust_llvm() && llvm_util::get_version() < (14, 0, 0) { + llvm::Attribute::NoInline.apply_callsite(llvm::AttributePlace::Function, llret); + } } } diff --git a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs index 6c911938ccc09..324823434ff77 100644 --- a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs @@ -1902,6 +1902,8 @@ extern "C" { pub fn LLVMRustVersionMinor() -> u32; pub fn LLVMRustVersionPatch() -> u32; + pub fn LLVMRustIsRustLLVM() -> bool; + pub fn LLVMRustAddModuleFlag(M: &Module, name: *const c_char, value: u32); pub fn LLVMRustMetadataAsValue<'a>(C: &'a Context, MD: &'a Metadata) -> &'a Value; diff --git a/compiler/rustc_codegen_llvm/src/llvm_util.rs b/compiler/rustc_codegen_llvm/src/llvm_util.rs index e4935ac431c8d..7e9850c23cdb5 100644 --- a/compiler/rustc_codegen_llvm/src/llvm_util.rs +++ b/compiler/rustc_codegen_llvm/src/llvm_util.rs @@ -217,6 +217,12 @@ pub fn get_version() -> (u32, u32, u32) { } } +/// Returns `true` if this LLVM is Rust's bundled LLVM (and not system LLVM). +pub fn is_rust_llvm() -> bool { + // Can be called without initializing LLVM + unsafe { llvm::LLVMRustIsRustLLVM() } +} + pub fn print_passes() { // Can be called without initializing LLVM unsafe { diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index 8cefd415e28b8..dcfe5fcc2ca73 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -160,8 +160,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. - bx.mark_callsite_cold(llret); + bx.apply_attrs_to_cleanup_callsite(llret); } if let Some((ret_dest, target)) = destination { diff --git a/compiler/rustc_codegen_ssa/src/traits/builder.rs b/compiler/rustc_codegen_ssa/src/traits/builder.rs index c1cdff3addadd..48d88095855d1 100644 --- a/compiler/rustc_codegen_ssa/src/traits/builder.rs +++ b/compiler/rustc_codegen_ssa/src/traits/builder.rs @@ -311,5 +311,5 @@ pub trait BuilderMethods<'a, 'tcx>: ) -> Self::Value; fn zext(&mut self, val: Self::Value, dest_ty: Self::Type) -> Self::Value; - fn mark_callsite_cold(&mut self, llret: Self::Value); + fn apply_attrs_to_cleanup_callsite(&mut self, llret: Self::Value); } diff --git a/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp b/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp index 3fbf020c552d7..025a0ee376e3e 100644 --- a/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp +++ b/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp @@ -716,6 +716,14 @@ extern "C" uint32_t LLVMRustVersionMinor() { return LLVM_VERSION_MINOR; } extern "C" uint32_t LLVMRustVersionMajor() { return LLVM_VERSION_MAJOR; } +extern "C" bool LLVMRustIsRustLLVM() { +#ifdef LLVM_RUSTLLVM + return true; +#else + return false; +#endif +} + extern "C" void LLVMRustAddModuleFlag(LLVMModuleRef M, const char *Name, uint32_t Value) { unwrap(M)->addModuleFlag(Module::Warning, Name, Value); diff --git a/src/test/codegen/unwind-landingpad-cold.rs b/src/test/codegen/unwind-landingpad-cold.rs index 0bf2941374a66..650d5b230f4c2 100644 --- a/src/test/codegen/unwind-landingpad-cold.rs +++ b/src/test/codegen/unwind-landingpad-cold.rs @@ -1,3 +1,4 @@ +// no-system-llvm: needs #92110 // compile-flags: -Cno-prepopulate-passes #![crate_type = "lib"] diff --git a/src/test/codegen/unwind-landingpad-inline.rs b/src/test/codegen/unwind-landingpad-inline.rs index d641c908ea99a..4f13b60b1f9ef 100644 --- a/src/test/codegen/unwind-landingpad-inline.rs +++ b/src/test/codegen/unwind-landingpad-inline.rs @@ -1,4 +1,4 @@ -// no-system-llvm: needs patch for Rust alloc/dealloc functions +// no-system-llvm: needs #92110 + patch for Rust alloc/dealloc functions // compile-flags: -Copt-level=3 #![crate_type = "lib"] From 64da730a41ac7df5fa8e03a82beb50abcdf5123e Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Thu, 30 Dec 2021 01:03:17 -0500 Subject: [PATCH 3/3] add test for noop drop in landing pad --- src/test/codegen/unwind-landingpad-inline.rs | 21 +++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/test/codegen/unwind-landingpad-inline.rs b/src/test/codegen/unwind-landingpad-inline.rs index 4f13b60b1f9ef..ce78d075dd0f4 100644 --- a/src/test/codegen/unwind-landingpad-inline.rs +++ b/src/test/codegen/unwind-landingpad-inline.rs @@ -3,11 +3,12 @@ #![crate_type = "lib"] // This test checks that we can inline drop_in_place in -// unwind landing pads. Without this, the box pointers escape, +// 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 - -// Everything should be optimized out. // CHECK-LABEL: @check_no_escape_in_landingpad // CHECK: start: // CHECK-NEXT: ret void @@ -20,3 +21,17 @@ pub fn check_no_escape_in_landingpad(f: fn()) { 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(); +}