From b91d211b40300a3c026b330e50a6e3e19d71351c Mon Sep 17 00:00:00 2001 From: Peter Jin Date: Mon, 31 Dec 2018 10:58:13 -0800 Subject: [PATCH] Add a target option "merge-functions" taking values in ("disabled", "trampolines", or "aliases (the default)) to allow targets to opt out of the MergeFunctions LLVM pass. Also add a corresponding -Z option with the same name and values. This works around: https://github.com/rust-lang/rust/issues/57356 Motivation: Basically, the problem is that the MergeFunctions pass, which rustc currently enables by default at -O2 and -O3, and `extern "ptx-kernel"` functions (specific to the NVPTX target) are currently not compatible with each other. If the MergeFunctions pass is allowed to run, rustc can generate invalid PTX assembly (i.e. a PTX file that is not accepted by the native PTX assembler ptxas). Therefore we would like a way to opt out of the MergeFunctions pass, which is what our target option does. Related work: The current behavior of rustc is to enable MergeFunctions at -O2 and -O3, and also to enable the use of function aliases within MergeFunctions. MergeFunctions both with and without function aliases is incompatible with the NVPTX target. clang's "solution" is to have a "-fmerge-functions" flag that opts in to the MergeFunctions pass, but it is not enabled by default. --- src/librustc/session/config.rs | 28 +++++++++-- src/librustc_codegen_llvm/llvm_util.rs | 10 +++- src/librustc_codegen_ssa/back/write.rs | 21 +++++++- src/librustc_target/spec/mod.rs | 66 +++++++++++++++++++++++++- 4 files changed, 117 insertions(+), 8 deletions(-) diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 33409f9b4a74f..ca4ab15d79b1d 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -6,7 +6,7 @@ use std::str::FromStr; use session::{early_error, early_warn, Session}; use session::search_paths::SearchPath; -use rustc_target::spec::{LinkerFlavor, PanicStrategy, RelroLevel}; +use rustc_target::spec::{LinkerFlavor, MergeFunctions, PanicStrategy, RelroLevel}; use rustc_target::spec::{Target, TargetTriple}; use lint; use middle::cstore; @@ -808,13 +808,16 @@ macro_rules! options { pub const parse_cross_lang_lto: Option<&str> = Some("either a boolean (`yes`, `no`, `on`, `off`, etc), \ or the path to the linker plugin"); + pub const parse_merge_functions: Option<&str> = + Some("one of: `disabled`, `trampolines`, or `aliases`"); } #[allow(dead_code)] mod $mod_set { use super::{$struct_name, Passes, Sanitizer, LtoCli, CrossLangLto}; - use rustc_target::spec::{LinkerFlavor, PanicStrategy, RelroLevel}; + use rustc_target::spec::{LinkerFlavor, MergeFunctions, PanicStrategy, RelroLevel}; use std::path::PathBuf; + use std::str::FromStr; $( pub fn $opt(cg: &mut $struct_name, v: Option<&str>) -> bool { @@ -1046,6 +1049,14 @@ macro_rules! options { }; true } + + fn parse_merge_functions(slot: &mut Option, v: Option<&str>) -> bool { + match v.and_then(|s| MergeFunctions::from_str(s).ok()) { + Some(mergefunc) => *slot = Some(mergefunc), + _ => return false, + } + true + } } ) } @@ -1380,6 +1391,9 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "whether to use the PLT when calling into shared libraries; only has effect for PIC code on systems with ELF binaries (default: PLT is disabled if full relro is enabled)"), + merge_functions: Option = (None, parse_merge_functions, [TRACKED], + "control the operation of the MergeFunctions LLVM pass, taking + the same values as the target option of the same name"), } pub fn default_lib_output() -> CrateType { @@ -2398,7 +2412,7 @@ mod dep_tracking { use super::{CrateType, DebugInfo, ErrorOutputType, OptLevel, OutputTypes, Passes, Sanitizer, LtoCli, CrossLangLto}; use syntax::feature_gate::UnstableFeatures; - use rustc_target::spec::{PanicStrategy, RelroLevel, TargetTriple}; + use rustc_target::spec::{MergeFunctions, PanicStrategy, RelroLevel, TargetTriple}; use syntax::edition::Edition; pub trait DepTrackingHash { @@ -2441,12 +2455,14 @@ mod dep_tracking { impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option<(String, u64)>); + impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(Option); impl_dep_tracking_hash_via_hash!(CrateType); + impl_dep_tracking_hash_via_hash!(MergeFunctions); impl_dep_tracking_hash_via_hash!(PanicStrategy); impl_dep_tracking_hash_via_hash!(RelroLevel); impl_dep_tracking_hash_via_hash!(Passes); @@ -2532,7 +2548,7 @@ mod tests { use std::iter::FromIterator; use std::path::PathBuf; use super::{Externs, OutputType, OutputTypes}; - use rustc_target::spec::{PanicStrategy, RelroLevel}; + use rustc_target::spec::{MergeFunctions, PanicStrategy, RelroLevel}; use syntax::symbol::Symbol; use syntax::edition::{Edition, DEFAULT_EDITION}; use syntax; @@ -3187,6 +3203,10 @@ mod tests { opts = reference.clone(); opts.debugging_opts.cross_lang_lto = CrossLangLto::LinkerPluginAuto; assert!(reference.dep_tracking_hash() != opts.dep_tracking_hash()); + + opts = reference.clone(); + opts.debugging_opts.merge_functions = Some(MergeFunctions::Disabled); + assert!(reference.dep_tracking_hash() != opts.dep_tracking_hash()); } #[test] diff --git a/src/librustc_codegen_llvm/llvm_util.rs b/src/librustc_codegen_llvm/llvm_util.rs index ad9ebbcde8ab9..dc70ebcf943a5 100644 --- a/src/librustc_codegen_llvm/llvm_util.rs +++ b/src/librustc_codegen_llvm/llvm_util.rs @@ -3,6 +3,7 @@ use back::write::create_target_machine; use llvm; use rustc::session::Session; use rustc::session::config::PrintRequest; +use rustc_target::spec::MergeFunctions; use libc::c_int; use std::ffi::CString; use syntax::feature_gate::UnstableFeatures; @@ -61,7 +62,14 @@ unsafe fn configure_llvm(sess: &Session) { add("-disable-preinline"); } if llvm::LLVMRustIsRustLLVM() { - add("-mergefunc-use-aliases"); + match sess.opts.debugging_opts.merge_functions + .unwrap_or(sess.target.target.options.merge_functions) { + MergeFunctions::Disabled | + MergeFunctions::Trampolines => {} + MergeFunctions::Aliases => { + add("-mergefunc-use-aliases"); + } + } } // HACK(eddyb) LLVM inserts `llvm.assume` calls to preserve align attributes diff --git a/src/librustc_codegen_ssa/back/write.rs b/src/librustc_codegen_ssa/back/write.rs index fb3e7ea696363..39bdc70f8322e 100644 --- a/src/librustc_codegen_ssa/back/write.rs +++ b/src/librustc_codegen_ssa/back/write.rs @@ -24,6 +24,7 @@ use rustc_fs_util::link_or_copy; use rustc_data_structures::svh::Svh; use rustc_errors::{Handler, Level, DiagnosticBuilder, FatalError, DiagnosticId}; use rustc_errors::emitter::{Emitter}; +use rustc_target::spec::MergeFunctions; use syntax::attr; use syntax::ext::hygiene::Mark; use syntax_pos::MultiSpan; @@ -152,8 +153,24 @@ impl ModuleConfig { sess.opts.optimize == config::OptLevel::Aggressive && !sess.target.target.options.is_like_emscripten; - self.merge_functions = sess.opts.optimize == config::OptLevel::Default || - sess.opts.optimize == config::OptLevel::Aggressive; + // Some targets (namely, NVPTX) interact badly with the MergeFunctions + // pass. This is because MergeFunctions can generate new function calls + // which may interfere with the target calling convention; e.g. for the + // NVPTX target, PTX kernels should not call other PTX kernels. + // MergeFunctions can also be configured to generate aliases instead, + // but aliases are not supported by some backends (again, NVPTX). + // Therefore, allow targets to opt out of the MergeFunctions pass, + // but otherwise keep the pass enabled (at O2 and O3) since it can be + // useful for reducing code size. + self.merge_functions = match sess.opts.debugging_opts.merge_functions + .unwrap_or(sess.target.target.options.merge_functions) { + MergeFunctions::Disabled => false, + MergeFunctions::Trampolines | + MergeFunctions::Aliases => { + sess.opts.optimize == config::OptLevel::Default || + sess.opts.optimize == config::OptLevel::Aggressive + } + }; } pub fn bitcode_needed(&self) -> bool { diff --git a/src/librustc_target/spec/mod.rs b/src/librustc_target/spec/mod.rs index f42b0a1c3c958..3a21ca19b176b 100644 --- a/src/librustc_target/spec/mod.rs +++ b/src/librustc_target/spec/mod.rs @@ -217,6 +217,46 @@ impl ToJson for RelroLevel { } } +#[derive(Clone, Copy, Debug, PartialEq, Hash, RustcEncodable, RustcDecodable)] +pub enum MergeFunctions { + Disabled, + Trampolines, + Aliases +} + +impl MergeFunctions { + pub fn desc(&self) -> &str { + match *self { + MergeFunctions::Disabled => "disabled", + MergeFunctions::Trampolines => "trampolines", + MergeFunctions::Aliases => "aliases", + } + } +} + +impl FromStr for MergeFunctions { + type Err = (); + + fn from_str(s: &str) -> Result { + match s { + "disabled" => Ok(MergeFunctions::Disabled), + "trampolines" => Ok(MergeFunctions::Trampolines), + "aliases" => Ok(MergeFunctions::Aliases), + _ => Err(()), + } + } +} + +impl ToJson for MergeFunctions { + fn to_json(&self) -> Json { + match *self { + MergeFunctions::Disabled => "disabled".to_json(), + MergeFunctions::Trampolines => "trampolines".to_json(), + MergeFunctions::Aliases => "aliases".to_json(), + } + } +} + pub type LinkArgs = BTreeMap>; pub type TargetResult = Result; @@ -690,7 +730,15 @@ pub struct TargetOptions { /// If set, have the linker export exactly these symbols, instead of using /// the usual logic to figure this out from the crate itself. - pub override_export_symbols: Option> + pub override_export_symbols: Option>, + + /// Determines how or whether the MergeFunctions LLVM pass should run for + /// this target. Either "disabled", "trampolines", or "aliases". + /// The MergeFunctions pass is generally useful, but some targets may need + /// to opt out. The default is "aliases". + /// + /// Workaround for: https://github.com/rust-lang/rust/issues/57356 + pub merge_functions: MergeFunctions } impl Default for TargetOptions { @@ -773,6 +821,7 @@ impl Default for TargetOptions { requires_uwtable: false, simd_types_indirect: true, override_export_symbols: None, + merge_functions: MergeFunctions::Aliases, } } } @@ -875,6 +924,19 @@ impl Target { .map(|o| o.as_u64() .map(|s| base.options.$key_name = Some(s))); } ); + ($key_name:ident, MergeFunctions) => ( { + let name = (stringify!($key_name)).replace("_", "-"); + obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { + match s.parse::() { + Ok(mergefunc) => base.options.$key_name = mergefunc, + _ => return Some(Err(format!("'{}' is not a valid value for \ + merge-functions. Use 'disabled', \ + 'trampolines', or 'aliases'.", + s))), + } + Some(Ok(())) + })).unwrap_or(Ok(())) + } ); ($key_name:ident, PanicStrategy) => ( { let name = (stringify!($key_name)).replace("_", "-"); obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| { @@ -1064,6 +1126,7 @@ impl Target { key!(requires_uwtable, bool); key!(simd_types_indirect, bool); key!(override_export_symbols, opt_list); + key!(merge_functions, MergeFunctions)?; if let Some(array) = obj.find("abi-blacklist").and_then(Json::as_array) { for name in array.iter().filter_map(|abi| abi.as_string()) { @@ -1275,6 +1338,7 @@ impl ToJson for Target { target_option_val!(requires_uwtable); target_option_val!(simd_types_indirect); target_option_val!(override_export_symbols); + target_option_val!(merge_functions); if default.abi_blacklist != self.options.abi_blacklist { d.insert("abi-blacklist".to_string(), self.options.abi_blacklist.iter()