Skip to content

Commit

Permalink
Code review feedback.
Browse files Browse the repository at this point in the history
* Removed `Config::cranelift_clear_cpu_flags`.
* Renamed `Config::cranelift_other_flag` to `Config::cranelift::flag_set`.
* Renamed `--cranelift-flag` to `--cranelift-set`.
* Renamed `--cranelift-preset` to `--cranelift-enable`.
  • Loading branch information
peterhuene committed Apr 1, 2021
1 parent 1eb5e90 commit ea0d569
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 44 deletions.
17 changes: 14 additions & 3 deletions RELEASES.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,27 @@

* The `Module::compile` method was added to support AOT compilation of a module.

* Added the `Config::cranelift_flag_enable` to enable setting Cranelift boolean
flags or presets in a config.
* Added the `Config::target` method to change the compilation target of the
configuration. This can be used in conjunction with `Module::compile` to target
a different host triple than the current one.

* Added the `Config::cranelift_flag_enable` method to enable setting Cranelift
boolean flags or presets in a config.

* Added CLI option `--cranelift-enable` to enable boolean settings and ISA presets.

### Changed

* Wasmtime CLI options to enable WebAssembly features have been replaced with a
singular `--wasm-features` option. The previous options are still supported, but
are not displayed in help text.

* Breaking: the CLI option `--cranelift-flags` was changed to `--cranelift-flag`.
* Breaking: `Config::cranelift_clear_cpu_flags` was removed. Use `Config::target`
to clear the CPU flags for the host's target.

* Breaking: `Config::cranelift_other_flag` was renamed to `Config::cranelift_flag_set`.

* Breaking: the CLI option `--cranelift-flags` was changed to `--cranelift-set`.

* Breaking: the CLI option `--enable-reference-types=false` has been changed to
`--wasm-features=-reference-types`.
Expand Down
24 changes: 3 additions & 21 deletions crates/wasmtime/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -441,10 +441,8 @@ impl Config {
///
/// This method can be used to change the target triple.
///
/// Note that any no Cranelift flags will be inferred for the given target.
///
/// [`Config::cranelift_clear_cpu_flags`] will reset the target triple back to
/// the host's target.
/// Cranelift flags will not be inferred for the given target and any
/// existing target-specific Cranelift flags will be cleared.
///
/// # Errors
///
Expand Down Expand Up @@ -898,22 +896,6 @@ impl Config {
self
}

/// Clears native CPU flags inferred from the host.
///
/// Note: this method will change the target to that of the host.
///
/// By default Wasmtime will tune generated code for the host that Wasmtime
/// itself is running on. If you're compiling on one host, however, and
/// shipping artifacts to another host then this behavior may not be
/// desired. This function will clear all inferred native CPU features.
///
/// To enable CPU features afterwards it's recommended to use the
/// [`Config::cranelift_other_flag`] method.
pub fn cranelift_clear_cpu_flags(&mut self) -> &mut Self {
self.isa_flags = native::builder_without_flags();
self
}

/// Allows setting a Cranelift boolean flag or preset. This allows
/// fine-tuning of Cranelift settings.
///
Expand Down Expand Up @@ -954,7 +936,7 @@ impl Config {
///
/// This method can fail if the flag's name does not exist, or the value is not appropriate for
/// the flag type.
pub unsafe fn cranelift_other_flag(&mut self, name: &str, value: &str) -> Result<&mut Self> {
pub unsafe fn cranelift_flag_set(&mut self, name: &str, value: &str) -> Result<&mut Self> {
if let Err(err) = self.flags.set(name, value) {
match err {
SetError::BadName(_) => {
Expand Down
28 changes: 11 additions & 17 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,12 +213,12 @@ struct CommonOptions {
opt_level: Option<wasmtime::OptLevel>,

/// Cranelift common flags to set.
#[structopt(long = "cranelift-flag", value_name = "NAME=VALUE", parse(try_from_str = parse_cranelift_flag))]
cranelift_flags: Vec<CraneliftFlag>,
#[structopt(long = "cranelift-set", value_name = "NAME=VALUE", parse(try_from_str = parse_cranelift_flag))]
cranelift_set: Vec<(String, String)>,

/// The Cranelift ISA preset to use.
#[structopt(long, value_name = "PRESET")]
cranelift_preset: Option<String>,
/// The Cranelift boolean setting or preset to enable.
#[structopt(long, value_name = "SETTING")]
cranelift_enable: Vec<String>,

/// Maximum size in bytes of wasm memory before it becomes dynamically
/// relocatable instead of up-front-reserved.
Expand Down Expand Up @@ -273,15 +273,15 @@ impl CommonOptions {

self.enable_wasm_features(&mut config);

if let Some(preset) = &self.cranelift_preset {
for name in &self.cranelift_enable {
unsafe {
config.cranelift_flag_enable(preset)?;
config.cranelift_flag_enable(name)?;
}
}

for CraneliftFlag { name, value } in &self.cranelift_flags {
for (name, value) in &self.cranelift_set {
unsafe {
config.cranelift_other_flag(name, value)?;
config.cranelift_flag_set(name, value)?;
}
}

Expand Down Expand Up @@ -401,13 +401,7 @@ fn parse_wasm_features(features: &str) -> Result<wasmparser::WasmFeatures> {
memory64: false,
})
}

struct CraneliftFlag {
name: String,
value: String,
}

fn parse_cranelift_flag(name_and_value: &str) -> Result<CraneliftFlag> {
fn parse_cranelift_flag(name_and_value: &str) -> Result<(String, String)> {
let mut split = name_and_value.splitn(2, '=');
let name = if let Some(name) = split.next() {
name.to_string()
Expand All @@ -419,7 +413,7 @@ fn parse_cranelift_flag(name_and_value: &str) -> Result<CraneliftFlag> {
} else {
bail!("missing value in cranelift flag");
};
Ok(CraneliftFlag { name, value })
Ok((name, value))
}

fn parse_target(s: &str) -> Result<Triple> {
Expand Down
11 changes: 8 additions & 3 deletions tests/all/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,27 @@ fn caches_across_engines() {

// differ in shared cranelift flags
let res = Module::deserialize(
&Engine::new(&Config::new().cranelift_nan_canonicalization(true)).unwrap(),
&Engine::new(Config::new().cranelift_nan_canonicalization(true)).unwrap(),
&bytes,
);
assert!(res.is_err());

// differ in cranelift settings
let res = Module::deserialize(
&Engine::new(&Config::new().cranelift_opt_level(OptLevel::None)).unwrap(),
&Engine::new(Config::new().cranelift_opt_level(OptLevel::None)).unwrap(),
&bytes,
);
assert!(res.is_err());

// Missing required cpu flags
if cfg!(target_arch = "x86_64") {
let res = Module::deserialize(
&Engine::new(&Config::new().cranelift_clear_cpu_flags()).unwrap(),
&Engine::new(
Config::new()
.target(&target_lexicon::Triple::host().to_string())
.unwrap(),
)
.unwrap(),
&bytes,
);
assert!(res.is_err());
Expand Down

0 comments on commit ea0d569

Please sign in to comment.