Rollup merge of rust-lang#57268 - peterhj:peterhj-optmergefunc, r=nagisa

Centril · web-flow · commit 63d710d0c581 · 2019-01-17T03:48:36.000+01:00
Add a target option "merge-functions", and a corresponding -Z flag (works around rust-lang#57356) This commit adds a target option "merge-functions", which takes values in ("disabled", "trampolines", or "aliases" (default is "aliases")), to allow targets to opt out of the MergeFunctions LLVM pass. Additionally, the latest commit also adds an optional -Z flag, "merge-functions", which takes the same values and has precedence over the target option when both are specified. This works around rust-lang#57356. cc @eddyb @japaric @oli-obk @nox @nagisa Also thanks to @denzp and @gnzlbg for discussing this on rust-cuda! ### Motivation Basically, the problem is that the MergeFunctions pass, which rustc currently enables by default at -O2 and -O3 [1], 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 [1], and also to enable the use of function aliases within MergeFunctions [2] [3]. MergeFunctions seems to have some benefits, such as reducing code size and fixing a crash [4], which is why it is enabled. However, MergeFunctions both with and without function aliases is incompatible with the NVPTX target; a more detailed example for both cases is given below. clang's "solution" is to have a "-fmerge-functions" flag that opts in to the MergeFunctions pass, but it is not enabled by default. ### Examples/more details Consider an example Rust lib using `extern "ptx-kernel"` functions: https://github.com/peterhj/nvptx-mergefunc-bug/blob/master/nocore.rs. If we try to compile this with nightly rustc, we get the following compiler error: LLVM ERROR: Module has aliases, which NVPTX does not support. This error happens because: (1) functions `foo` and `bar` have the same body, so are candidates to be merged by MergeFunctions; and (2) rustc configures MergeFunctions to generate function aliases using the "mergefunc-use-aliases" LLVM option [2] [3], but the NVPTX backend does not support those aliases. Okay, so we can try omitting "mergefunc-use-aliases", and then rustc will happily emit PTX assembly: https://github.com/peterhj/nvptx-mergefunc-bug/blob/master/nocore-mergefunc-nousealiases-bad.ptx. However, this PTX is invalid! When we try to assemble it with `ptxas` (I'm on the CUDA 9.2 toolchain), we get an assembler error: ptxas nocore-mergefunc-nousealiases-bad.ptx, line 38; error : Illegal call target, device function expected ptxas fatal : Ptx assembly aborted due to errors What's happening is that MergeFunctions rewrites the `bar` function to call `foo`. However, directly calling an `extern "ptx-kernel"` function from another `extern "ptx-kernel"` is wrong. If we disable the MergeFunctions pass from running at all, rustc generates correct PTX assembly: https://github.com/peterhj/nvptx-mergefunc-bug/blob/master/nocore-nomergefunc-ok.ptx [1] https://github.com/rust-lang/rust/blob/a36b960df626cbb8bea74f01243318b73f0bd201/src/librustc_codegen_ssa/back/write.rs#L155 [2] https://github.com/rust-lang/rust/blob/a36b960df626cbb8bea74f01243318b73f0bd201/src/librustc_codegen_llvm/llvm_util.rs#L64 [3] rust-lang#56358 [4] rust-lang#49479
diff --git 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<MergeFunctions>, v: Option<&str>) -> bool {
+            match v.and_then(|s| MergeFunctions::from_str(s).ok()) {
+                Some(mergefunc) => *slot = Some(mergefunc),
+                _ => return false,
+            }
+            true
+        }
     }
 ) }
 
@@ -1376,6 +1387,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<MergeFunctions> = (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 {
@@ -2394,7 +2408,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 {
@@ -2437,12 +2451,14 @@ mod dep_tracking {
     impl_dep_tracking_hash_via_hash!(Option<usize>);
     impl_dep_tracking_hash_via_hash!(Option<String>);
     impl_dep_tracking_hash_via_hash!(Option<(String, u64)>);
+    impl_dep_tracking_hash_via_hash!(Option<MergeFunctions>);
     impl_dep_tracking_hash_via_hash!(Option<PanicStrategy>);
     impl_dep_tracking_hash_via_hash!(Option<RelroLevel>);
     impl_dep_tracking_hash_via_hash!(Option<lint::Level>);
     impl_dep_tracking_hash_via_hash!(Option<PathBuf>);
     impl_dep_tracking_hash_via_hash!(Option<cstore::NativeLibraryKind>);
     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);
@@ -2528,7 +2544,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;
@@ -3183,6 +3199,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
@@ -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
@@ -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
@@ -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<MergeFunctions, ()> {
+        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<LinkerFlavor, Vec<String>>;
 pub type TargetResult = Result<Target, String>;
 
@@ -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<Vec<String>>
+    pub override_export_symbols: Option<Vec<String>>,
+
+    /// 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::<MergeFunctions>() {
+                        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()
