Skip to content

Commit

Permalink
Add a target option "merge-functions" taking values in ("disabled",
Browse files Browse the repository at this point in the history
"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: rust-lang#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.
  • Loading branch information
peterhj committed Jan 5, 2019
1 parent 2442823 commit b91d211
Show file tree
Hide file tree
Showing 4 changed files with 117 additions and 8 deletions.
28 changes: 24 additions & 4 deletions src/librustc/session/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
}
) }

Expand Down Expand Up @@ -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<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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -2441,12 +2455,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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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]
Expand Down
10 changes: 9 additions & 1 deletion src/librustc_codegen_llvm/llvm_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
21 changes: 19 additions & 2 deletions src/librustc_codegen_ssa/back/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
66 changes: 65 additions & 1 deletion src/librustc_target/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>;

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -773,6 +821,7 @@ impl Default for TargetOptions {
requires_uwtable: false,
simd_types_indirect: true,
override_export_symbols: None,
merge_functions: MergeFunctions::Aliases,
}
}
}
Expand Down Expand Up @@ -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| {
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit b91d211

Please sign in to comment.