Skip to content

Commit

Permalink
Cranelift: ensure ISA level needed for SIMD is present when SIMD is e…
Browse files Browse the repository at this point in the history
…nabled.

Addresses #3809: when we are asked to create a Cranelift backend with
shared flags that indicate support for SIMD, we should check that the
ISA level needed for our SIMD lowerings is present.
  • Loading branch information
cfallin committed Feb 16, 2022
1 parent db9e3ce commit a218fb6
Show file tree
Hide file tree
Showing 8 changed files with 53 additions and 16 deletions.
2 changes: 1 addition & 1 deletion cranelift/codegen/src/isa/aarch64/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ pub fn isa_builder(triple: Triple) -> IsaBuilder {
constructor: |triple, shared_flags, builder| {
let isa_flags = aarch64_settings::Flags::new(&shared_flags, builder);
let backend = AArch64Backend::new_with_flags(triple, shared_flags, isa_flags);
Box::new(backend)
Ok(Box::new(backend))
},
}
}
Expand Down
13 changes: 9 additions & 4 deletions cranelift/codegen/src/isa/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,8 @@ impl fmt::Display for LookupError {
pub struct Builder {
triple: Triple,
setup: settings::Builder,
constructor: fn(Triple, settings::Flags, settings::Builder) -> Box<dyn TargetIsa>,
constructor:
fn(Triple, settings::Flags, settings::Builder) -> CodegenResult<Box<dyn TargetIsa>>,
}

impl Builder {
Expand All @@ -153,9 +154,13 @@ impl Builder {
self.setup.iter()
}

/// Combine the ISA-specific settings with the provided ISA-independent settings and allocate a
/// fully configured `TargetIsa` trait object.
pub fn finish(self, shared_flags: settings::Flags) -> Box<dyn TargetIsa> {
/// Combine the ISA-specific settings with the provided
/// ISA-independent settings and allocate a fully configured
/// `TargetIsa` trait object. May return an error if some of the
/// flags are inconsistent or incompatible: for example, some
/// platform-independent features, like general SIMD support, may
/// need certain ISA extensions to be enabled.
pub fn finish(self, shared_flags: settings::Flags) -> CodegenResult<Box<dyn TargetIsa>> {
(self.constructor)(self.triple, shared_flags, self.setup)
}
}
Expand Down
2 changes: 1 addition & 1 deletion cranelift/codegen/src/isa/s390x/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ pub fn isa_builder(triple: Triple) -> IsaBuilder {
constructor: |triple, shared_flags, builder| {
let isa_flags = s390x_settings::Flags::new(&shared_flags, builder);
let backend = S390xBackend::new_with_flags(triple, shared_flags, isa_flags);
Box::new(backend)
Ok(Box::new(backend))
},
}
}
Expand Down
6 changes: 4 additions & 2 deletions cranelift/codegen/src/isa/x64/inst/unwind/systemv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ mod tests {
fn test_simple_func() {
let isa = lookup(triple!("x86_64"))
.expect("expect x86 ISA")
.finish(Flags::new(builder()));
.finish(Flags::new(builder()))
.expect("expect backend creation to succeed");

let mut context = Context::for_function(create_function(
CallConv::SystemV,
Expand Down Expand Up @@ -154,7 +155,8 @@ mod tests {
fn test_multi_return_func() {
let isa = lookup(triple!("x86_64"))
.expect("expect x86 ISA")
.finish(Flags::new(builder()));
.finish(Flags::new(builder()))
.expect("expect backend creation to succeed");

let mut context = Context::for_function(create_multi_return_function(CallConv::SystemV));

Expand Down
33 changes: 30 additions & 3 deletions cranelift/codegen/src/isa/x64/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::isa::Builder as IsaBuilder;
use crate::machinst::{
compile, MachCompileResult, MachTextSectionBuilder, TextSectionBuilder, VCode,
};
use crate::result::CodegenResult;
use crate::result::{CodegenError, CodegenResult};
use crate::settings::{self as shared_settings, Flags};
use alloc::{boxed::Box, vec::Vec};
use core::fmt;
Expand Down Expand Up @@ -174,10 +174,21 @@ fn isa_constructor(
triple: Triple,
shared_flags: Flags,
builder: shared_settings::Builder,
) -> Box<dyn TargetIsa> {
) -> CodegenResult<Box<dyn TargetIsa>> {
let isa_flags = x64_settings::Flags::new(&shared_flags, builder);

// Check for compatibility between flags and ISA level
// requested. In particular, SIMD support requires SSE4.1.
if shared_flags.enable_simd() {
if !isa_flags.has_sse3() || !isa_flags.has_ssse3() || !isa_flags.has_sse41() {
return Err(CodegenError::Unsupported(
"SIMD support requires SSE3, SSSE3, and SSE4.1 on x86_64.".into(),
));
}
}

let backend = X64Backend::new_with_flags(triple, shared_flags, isa_flags);
Box::new(backend)
Ok(Box::new(backend))
}

#[cfg(test)]
Expand Down Expand Up @@ -332,4 +343,20 @@ mod test {

assert_eq!(code, &golden[..]);
}

// Check that feature tests for SIMD work correctly.
#[test]
fn simd_required_features() {
let mut shared_flags_builder = settings::builder();
shared_flags_builder.set("enable_simd", "true").unwrap();
let shared_flags = settings::Flags::new(shared_flags_builder);
let mut isa_builder = crate::isa::lookup_by_name("x86_64").unwrap();
isa_builder.set("has_sse3", "false").unwrap();
isa_builder.set("has_ssse3", "false").unwrap();
isa_builder.set("has_sse41", "false").unwrap();
assert!(matches!(
isa_builder.finish(shared_flags),
Err(CodegenError::Unsupported(_)),
));
}
}
9 changes: 6 additions & 3 deletions crates/cranelift/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,15 @@ impl CompilerBuilder for Builder {
Ok(())
}

fn build(&self) -> Box<dyn wasmtime_environ::Compiler> {
fn build(&self) -> Result<Box<dyn wasmtime_environ::Compiler>> {
let isa = self
.isa_flags
.clone()
.finish(settings::Flags::new(self.flags.clone()));
Box::new(crate::compiler::Compiler::new(isa, self.linkopts.clone()))
.finish(settings::Flags::new(self.flags.clone()))?;
Ok(Box::new(crate::compiler::Compiler::new(
isa,
self.linkopts.clone(),
)))
}

fn settings(&self) -> Vec<Setting> {
Expand Down
2 changes: 1 addition & 1 deletion crates/environ/src/compilation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ pub trait CompilerBuilder: Send + Sync + fmt::Debug {
fn settings(&self) -> Vec<Setting>;

/// Builds a new [`Compiler`] object from this configuration.
fn build(&self) -> Box<dyn Compiler>;
fn build(&self) -> Result<Box<dyn Compiler>>;
}

/// Description of compiler settings returned by [`CompilerBuilder::settings`].
Expand Down
2 changes: 1 addition & 1 deletion crates/wasmtime/src/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ impl Engine {
Ok(Engine {
inner: Arc::new(EngineInner {
#[cfg(compiler)]
compiler: config.compiler.build(),
compiler: config.compiler.build()?,
config,
allocator,
signatures: registry,
Expand Down

0 comments on commit a218fb6

Please sign in to comment.