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

Use RustOptimize to set optimize #112756

Merged
merged 3 commits into from
Jul 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
7 changes: 5 additions & 2 deletions src/bootstrap/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1207,7 +1207,7 @@ impl<'a> Builder<'a> {
assert_eq!(target, compiler.host);
}

if self.config.rust_optimize {
if self.config.rust_optimize.is_release() {
// FIXME: cargo bench/install do not accept `--release`
if cmd != "bench" && cmd != "install" {
cargo.arg("--release");
Expand Down Expand Up @@ -1263,7 +1263,7 @@ impl<'a> Builder<'a> {
}

let profile_var = |name: &str| {
let profile = if self.config.rust_optimize { "RELEASE" } else { "DEV" };
let profile = if self.config.rust_optimize.is_release() { "RELEASE" } else { "DEV" };
format!("CARGO_PROFILE_{}_{}", profile, name)
};

Expand Down Expand Up @@ -1652,6 +1652,9 @@ impl<'a> Builder<'a> {
}
};
cargo.env(profile_var("DEBUG"), debuginfo_level.to_string());
if let Some(opt_level) = &self.config.rust_optimize.get_opt_level() {
cargo.env(profile_var("OPT_LEVEL"), opt_level);
}
if !self.config.dry_run() && self.cc.borrow()[&target].args().iter().any(|arg| arg == "-gz")
{
rustflags.arg("-Clink-arg=-gz");
Expand Down
48 changes: 43 additions & 5 deletions src/bootstrap/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ pub struct Config {
pub llvm_use_libcxx: bool,

// rust codegen options
pub rust_optimize: bool,
pub rust_optimize: RustOptimize,
pub rust_codegen_units: Option<u32>,
pub rust_codegen_units_std: Option<u32>,
pub rust_debug_assertions: bool,
Expand Down Expand Up @@ -875,17 +875,55 @@ impl Default for StringOrBool {
}
}

#[derive(Clone, Debug, Deserialize, PartialEq, Eq)]
#[serde(untagged)]
pub enum RustOptimize {
#[serde(deserialize_with = "deserialize_and_validate_opt_level")]
String(String),
Bool(bool),
}

impl Default for RustOptimize {
fn default() -> RustOptimize {
RustOptimize::Bool(false)
}
}

fn deserialize_and_validate_opt_level<'de, D>(d: D) -> Result<String, D::Error>
where
D: serde::de::Deserializer<'de>,
{
let v = String::deserialize(d)?;
if ["0", "1", "2", "3", "s", "z"].iter().find(|x| **x == v).is_some() {
Ok(v)
} else {
Err(format!(r#"unrecognized option for rust optimize: "{}", expected one of "0", "1", "2", "3", "s", "z""#, v)).map_err(serde::de::Error::custom)
}
}

impl RustOptimize {
pub(crate) fn is_release(&self) -> bool {
if let RustOptimize::Bool(true) | RustOptimize::String(_) = &self { true } else { false }
}

pub(crate) fn get_opt_level(&self) -> Option<String> {
match &self {
RustOptimize::String(s) => Some(s.clone()),
RustOptimize::Bool(_) => None,
}
}
}

#[derive(Deserialize)]
#[serde(untagged)]
enum StringOrInt<'a> {
String(&'a str),
Int(i64),
}

define_config! {
/// TOML representation of how the Rust build is configured.
struct Rust {
optimize: Option<bool> = "optimize",
optimize: Option<RustOptimize> = "optimize",
debug: Option<bool> = "debug",
codegen_units: Option<u32> = "codegen-units",
codegen_units_std: Option<u32> = "codegen-units-std",
Expand Down Expand Up @@ -971,7 +1009,7 @@ impl Config {
config.ninja_in_file = true;
config.llvm_static_stdcpp = false;
config.backtrace = true;
config.rust_optimize = true;
config.rust_optimize = RustOptimize::Bool(true);
config.rust_optimize_tests = true;
config.submodules = None;
config.docs = true;
Expand Down Expand Up @@ -1546,7 +1584,7 @@ impl Config {
config.llvm_assertions = llvm_assertions.unwrap_or(false);
config.llvm_tests = llvm_tests.unwrap_or(false);
config.llvm_plugins = llvm_plugins.unwrap_or(false);
config.rust_optimize = optimize.unwrap_or(true);
config.rust_optimize = optimize.unwrap_or(RustOptimize::Bool(true));

let default = debug == Some(true);
config.rust_debug_assertions = debug_assertions.unwrap_or(default);
Expand Down
15 changes: 15 additions & 0 deletions src/bootstrap/config/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,3 +178,18 @@ fn profile_user_dist() {
}
Config::parse_inner(&["check".to_owned()], get_toml);
}

#[test]
fn rust_optimize() {
assert_eq!(parse("").rust_optimize.is_release(), true);
assert_eq!(parse("rust.optimize = false").rust_optimize.is_release(), false);
assert_eq!(parse("rust.optimize = true").rust_optimize.is_release(), true);
assert_eq!(parse("rust.optimize = \"1\"").rust_optimize.get_opt_level(), Some("1".to_string()));
Copy link
Member

@jyn514 jyn514 Jul 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd prefer to require this to be an int instead of a string, so rust.optimize = 1 instead of \"1\". no one is using the new config yet so i think we can break hard here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the "s" and "z"? Do you mean we need to use an IntOrString here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, exactly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix it later.

assert_eq!(parse("rust.optimize = \"s\"").rust_optimize.get_opt_level(), Some("s".to_string()));
}

#[test]
#[should_panic]
fn invalid_rust_optimize() {
parse("rust.optimize = \"a\"");
}
2 changes: 1 addition & 1 deletion src/bootstrap/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,7 @@ impl Build {
/// Component directory that Cargo will produce output into (e.g.
/// release/debug)
fn cargo_dir(&self) -> &'static str {
if self.config.rust_optimize { "release" } else { "debug" }
if self.config.rust_optimize.is_release() { "release" } else { "debug" }
}

fn tools_dir(&self, compiler: Compiler) -> PathBuf {
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -801,7 +801,7 @@ impl Step for Clippy {
cargo.arg("-p").arg("clippy_dev");
// clippy_dev gets confused if it can't find `clippy/Cargo.toml`
cargo.current_dir(&builder.src.join("src").join("tools").join("clippy"));
if builder.config.rust_optimize {
if builder.config.rust_optimize.is_release() {
cargo.env("PROFILE", "release");
} else {
cargo.env("PROFILE", "debug");
Expand Down