From 4d2a2cfae6c154d919082d49dd9a53bd5ed3f06e Mon Sep 17 00:00:00 2001 From: Afonso Bordado Date: Tue, 9 Aug 2022 22:54:25 +0100 Subject: [PATCH] cranelift: Use `cranelift-jit` in runtests (#4453) * cranelift: Use JIT in runtests Using `cranelift-jit` in run tests allows us to preform relocations and libcalls. This is important since some instruction lowerings fallback to libcall's when an extension is missing, or when it's too complicated to implement manually. This is also a first step to being able to test `call`'s between functions in the runtest suite. It should also make it easier to eventually test TLS relocations, symbol resolution and ABI's. Another benefit of this is that we also get to test the JIT more, since it now runs the runtests, and gets some fuzzing via `fuzzgen` (which uses the `SingleFunctionCompiler`). This change causes regressions in terms of runtime for the filetests. I haven't done any serious benchmarking but what I've been seeing is that it now takes about ~3 seconds to run the testsuite while it previously took around 2 seconds. * Add FMA tests for X86 --- Cargo.lock | 3 +- cranelift/filetests/Cargo.toml | 3 +- .../filetests/filetests/runtests/ceil.clif | 1 + .../filetests/filetests/runtests/floor.clif | 1 + .../filetests/runtests/fma-interpreter.clif | 4 +- .../filetests/filetests/runtests/nearest.clif | 1 + .../filetests/filetests/runtests/trunc.clif | 1 + cranelift/filetests/src/function_runner.rs | 173 +++++++++--------- cranelift/filetests/src/test_run.rs | 30 ++- cranelift/fuzzgen/src/function_generator.rs | 2 +- cranelift/jit/src/backend.rs | 4 +- cranelift/src/run.rs | 5 +- fuzz/fuzz_targets/cranelift-fuzzgen.rs | 2 +- 13 files changed, 121 insertions(+), 109 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3d6bfc50d1..aeee11bec4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -573,6 +573,8 @@ dependencies = [ "cranelift-codegen", "cranelift-frontend", "cranelift-interpreter", + "cranelift-jit", + "cranelift-module", "cranelift-native", "cranelift-preopt", "cranelift-reader", @@ -580,7 +582,6 @@ dependencies = [ "filecheck", "gimli", "log", - "memmap2", "num_cpus", "similar", "target-lexicon", diff --git a/cranelift/filetests/Cargo.toml b/cranelift/filetests/Cargo.toml index 47a2b49c59..1a2415766a 100644 --- a/cranelift/filetests/Cargo.toml +++ b/cranelift/filetests/Cargo.toml @@ -16,11 +16,12 @@ cranelift-interpreter = { path = "../interpreter", version = "0.88.0" } cranelift-native = { path = "../native", version = "0.88.0" } cranelift-reader = { path = "../reader", version = "0.88.0" } cranelift-preopt = { path = "../preopt", version = "0.88.0" } +cranelift-jit = { path = "../jit", version = "0.88.0" } +cranelift-module = { path = "../module", version = "0.88.0" } file-per-thread-logger = "0.1.2" filecheck = "0.5.0" gimli = { version = "0.26.0", default-features = false, features = ["read"] } log = "0.4.6" -memmap2 = "0.2.1" num_cpus = "1.8.0" target-lexicon = "0.12" thiserror = "1.0.15" diff --git a/cranelift/filetests/filetests/runtests/ceil.clif b/cranelift/filetests/filetests/runtests/ceil.clif index 8031c76d57..89f8abcf8f 100644 --- a/cranelift/filetests/filetests/runtests/ceil.clif +++ b/cranelift/filetests/filetests/runtests/ceil.clif @@ -1,6 +1,7 @@ test interpret test run target x86_64 +target x86_64 has_sse41=false target aarch64 target s390x diff --git a/cranelift/filetests/filetests/runtests/floor.clif b/cranelift/filetests/filetests/runtests/floor.clif index 5a15c8be8c..cbb67aad4f 100644 --- a/cranelift/filetests/filetests/runtests/floor.clif +++ b/cranelift/filetests/filetests/runtests/floor.clif @@ -1,6 +1,7 @@ test interpret test run target x86_64 +target x86_64 has_sse41=false target aarch64 target s390x diff --git a/cranelift/filetests/filetests/runtests/fma-interpreter.clif b/cranelift/filetests/filetests/runtests/fma-interpreter.clif index 90e3d566e8..c96c2a6e4f 100644 --- a/cranelift/filetests/filetests/runtests/fma-interpreter.clif +++ b/cranelift/filetests/filetests/runtests/fma-interpreter.clif @@ -1,8 +1,10 @@ test interpret +target x86_64 has_avx=false has_fma=false ; The interpreter can run `fma.clif` on most platforms, however on `x86_64-pc-windows-gnu` we ; use libm which has issues with some inputs. We should delete this file and enable the interpreter -; on the main `fma.clif` file once those are fixed. +; on the main `fma.clif` file once those are fixed. The same issue applies to x86 with fma disabled +; since it will call the native runtime's fma function. ; See: https://github.com/bytecodealliance/wasmtime/pull/4517 ; See: https://github.com/rust-lang/libm/issues/263 diff --git a/cranelift/filetests/filetests/runtests/nearest.clif b/cranelift/filetests/filetests/runtests/nearest.clif index dd265a18e2..cd2397e9ad 100644 --- a/cranelift/filetests/filetests/runtests/nearest.clif +++ b/cranelift/filetests/filetests/runtests/nearest.clif @@ -1,6 +1,7 @@ test interpret test run target x86_64 +target x86_64 has_sse41=false target aarch64 target s390x diff --git a/cranelift/filetests/filetests/runtests/trunc.clif b/cranelift/filetests/filetests/runtests/trunc.clif index 99410b6043..0de97280fb 100644 --- a/cranelift/filetests/filetests/runtests/trunc.clif +++ b/cranelift/filetests/filetests/runtests/trunc.clif @@ -1,6 +1,7 @@ test interpret test run target x86_64 +target x86_64 has_sse41=false target aarch64 target s390x diff --git a/cranelift/filetests/src/function_runner.rs b/cranelift/filetests/src/function_runner.rs index 62efa83024..ba1ca323e5 100644 --- a/cranelift/filetests/src/function_runner.rs +++ b/cranelift/filetests/src/function_runner.rs @@ -4,13 +4,12 @@ use core::mem; use cranelift_codegen::data_value::DataValue; use cranelift_codegen::ir::{condcodes::IntCC, Function, InstBuilder, Signature}; use cranelift_codegen::isa::TargetIsa; -use cranelift_codegen::{ir, settings, CodegenError, Context}; +use cranelift_codegen::{ir, settings, CodegenError}; use cranelift_frontend::{FunctionBuilder, FunctionBuilderContext}; +use cranelift_jit::{JITBuilder, JITModule}; +use cranelift_module::{FuncId, Linkage, Module, ModuleError}; use cranelift_native::builder_with_options; -use log::trace; -use memmap2::{Mmap, MmapMut}; use std::cmp::max; -use std::collections::HashMap; use thiserror::Error; /// Compile a single function. @@ -25,16 +24,18 @@ use thiserror::Error; /// ``` /// use cranelift_filetests::SingleFunctionCompiler; /// use cranelift_reader::parse_functions; +/// use cranelift_codegen::data_value::DataValue; /// /// let code = "test run \n function %add(i32, i32) -> i32 { block0(v0:i32, v1:i32): v2 = iadd v0, v1 return v2 }".into(); /// let func = parse_functions(code).unwrap().into_iter().nth(0).unwrap(); -/// let mut compiler = SingleFunctionCompiler::with_default_host_isa().unwrap(); +/// let compiler = SingleFunctionCompiler::with_default_host_isa().unwrap(); /// let compiled_func = compiler.compile(func).unwrap(); -/// println!("Address of compiled function: {:p}", compiled_func.as_ptr()); +/// +/// let returned = compiled_func.call(&vec![DataValue::I32(2), DataValue::I32(40)]); +/// assert_eq!(vec![DataValue::I32(42)], returned); /// ``` pub struct SingleFunctionCompiler { isa: Box, - trampolines: HashMap, } impl SingleFunctionCompiler { @@ -42,8 +43,7 @@ impl SingleFunctionCompiler { /// host machine, this [TargetIsa] must match the host machine's ISA (see /// [SingleFunctionCompiler::with_host_isa]). pub fn new(isa: Box) -> Self { - let trampolines = HashMap::new(); - Self { isa, trampolines } + Self { isa } } /// Build a [SingleFunctionCompiler] using the host machine's ISA and the passed flags. @@ -66,27 +66,47 @@ impl SingleFunctionCompiler { /// - compile the [Function] /// - compile a `Trampoline` for the [Function]'s signature (or used a cached `Trampoline`; /// this makes it possible to call functions when the signature is not known until runtime. - pub fn compile(&mut self, function: Function) -> Result { + pub fn compile(self, function: Function) -> Result { let signature = function.signature.clone(); if signature.call_conv != self.isa.default_call_conv() { return Err(CompilationError::InvalidTargetIsa); } - // Compile the function itself. - let code_page = compile(function, self.isa.as_ref())?; - - // Compile the trampoline to call it, if necessary (it may be cached). - let isa = self.isa.as_ref(); - let trampoline = self - .trampolines - .entry(signature.clone()) - .or_insert_with(|| { - let ir = make_trampoline(&signature, isa); - let code = compile(ir, isa).expect("failed to compile trampoline"); - Trampoline::new(code) - }); - - Ok(CompiledFunction::new(code_page, signature, trampoline)) + let trampoline = make_trampoline(&signature, self.isa.as_ref()); + + let builder = JITBuilder::with_isa(self.isa, cranelift_module::default_libcall_names()); + let mut module = JITModule::new(builder); + let mut ctx = module.make_context(); + + let name = format!("{}", function.name); + let func_id = module.declare_function(&name, Linkage::Local, &function.signature)?; + + // Build and declare the trampoline in the module + let trampoline_name = format!("{}", trampoline.name); + let trampoline_id = + module.declare_function(&trampoline_name, Linkage::Local, &trampoline.signature)?; + + // Define both functions + let func_signature = function.signature.clone(); + ctx.func = function; + module.define_function(func_id, &mut ctx)?; + module.clear_context(&mut ctx); + + ctx.func = trampoline; + module.define_function(trampoline_id, &mut ctx)?; + module.clear_context(&mut ctx); + + // Finalize the functions which we just defined, which resolves any + // outstanding relocations (patching in addresses, now that they're + // available). + module.finalize_definitions(); + + Ok(CompiledFunction::new( + module, + func_signature, + func_id, + trampoline_id, + )) } } @@ -100,34 +120,16 @@ pub enum CompilationError { /// Cranelift codegen error. #[error("Cranelift codegen error")] CodegenError(#[from] CodegenError), + /// Module Error + #[error("Module error")] + ModuleError(#[from] ModuleError), /// Memory mapping error. #[error("Memory mapping error")] IoError(#[from] std::io::Error), } -/// Contains the compiled code to move memory-allocated [DataValue]s to the correct location (e.g. -/// register, stack) dictated by the calling convention before calling a [CompiledFunction]. Without -/// this, it would be quite difficult to correctly place [DataValue]s since both the calling -/// convention and function signature are not known until runtime. See [make_trampoline] for the -/// Cranelift IR used to build this. -pub struct Trampoline { - page: Mmap, -} - -impl Trampoline { - /// Build a new [Trampoline]. - pub fn new(page: Mmap) -> Self { - Self { page } - } - - /// Return a pointer to the compiled code. - fn as_ptr(&self) -> *const u8 { - self.page.as_ptr() - } -} - /// Container for the compiled code of a [Function]. This wrapper allows users to call the compiled -/// function through the use of a [Trampoline]. +/// function through the use of a trampoline. /// /// ``` /// use cranelift_filetests::SingleFunctionCompiler; @@ -136,47 +138,62 @@ impl Trampoline { /// /// let code = "test run \n function %add(i32, i32) -> i32 { block0(v0:i32, v1:i32): v2 = iadd v0, v1 return v2 }".into(); /// let func = parse_functions(code).unwrap().into_iter().nth(0).unwrap(); -/// let mut compiler = SingleFunctionCompiler::with_default_host_isa().unwrap(); +/// let compiler = SingleFunctionCompiler::with_default_host_isa().unwrap(); /// let compiled_func = compiler.compile(func).unwrap(); /// /// let returned = compiled_func.call(&vec![DataValue::I32(2), DataValue::I32(40)]); /// assert_eq!(vec![DataValue::I32(42)], returned); /// ``` -pub struct CompiledFunction<'a> { - page: Mmap, +pub struct CompiledFunction { + /// We need to store this since it contains the underlying memory for the functions + /// Store it in an [Option] so that we can later drop it. + module: Option, signature: Signature, - trampoline: &'a Trampoline, + func_id: FuncId, + trampoline_id: FuncId, } -impl<'a> CompiledFunction<'a> { +impl CompiledFunction { /// Build a new [CompiledFunction]. - pub fn new(page: Mmap, signature: Signature, trampoline: &'a Trampoline) -> Self { + pub fn new( + module: JITModule, + signature: Signature, + func_id: FuncId, + trampoline_id: FuncId, + ) -> Self { Self { - page, + module: Some(module), signature, - trampoline, + func_id, + trampoline_id, } } - /// Return a pointer to the compiled code. - pub fn as_ptr(&self) -> *const u8 { - self.page.as_ptr() - } - - /// Call the [CompiledFunction], passing in [DataValue]s using a compiled [Trampoline]. + /// Call the [CompiledFunction], passing in [DataValue]s using a compiled trampoline. pub fn call(&self, arguments: &[DataValue]) -> Vec { let mut values = UnboxedValues::make_arguments(arguments, &self.signature); let arguments_address = values.as_mut_ptr(); - let function_address = self.as_ptr(); + + let module = self.module.as_ref().unwrap(); + let function_ptr = module.get_finalized_function(self.func_id); + let trampoline_ptr = module.get_finalized_function(self.trampoline_id); let callable_trampoline: fn(*const u8, *mut u128) -> () = - unsafe { mem::transmute(self.trampoline.as_ptr()) }; - callable_trampoline(function_address, arguments_address); + unsafe { mem::transmute(trampoline_ptr) }; + callable_trampoline(function_ptr, arguments_address); values.collect_returns(&self.signature) } } +impl Drop for CompiledFunction { + fn drop(&mut self) { + // Freeing the module's memory erases the compiled functions. + // This should be safe since their pointers never leave this struct. + unsafe { self.module.take().unwrap().free_memory() } + } +} + /// A container for laying out the [ValueData]s in memory in a way that the [Trampoline] can /// understand. struct UnboxedValues(Vec); @@ -231,32 +248,6 @@ impl UnboxedValues { } } -/// Compile a [Function] to its executable bytes in memory. -/// -/// This currently returns a [Mmap], a type from an external crate, so we wrap this up before -/// exposing it in public APIs. -fn compile(function: Function, isa: &dyn TargetIsa) -> Result { - // Set up the context. - let mut context = Context::new(); - context.func = function; - - // Compile and encode the result to machine code. - let compiled_code = context.compile(isa).map_err(|err| err.inner)?; - let mut code_page = MmapMut::map_anon(compiled_code.code_info().total_size as usize)?; - - code_page.copy_from_slice(compiled_code.code_buffer()); - - let code_page = code_page.make_exec()?; - trace!( - "Compiled function {} with signature {} at: {:p}", - context.func.name, - context.func.signature, - code_page.as_ptr() - ); - - Ok(code_page) -} - /// Build the Cranelift IR for moving the memory-allocated [DataValue]s to their correct location /// (e.g. register, stack) prior to calling a [CompiledFunction]. The [Function] returned by /// [make_trampoline] is compiled to a [Trampoline]. Note that this uses the [TargetIsa]'s default @@ -383,7 +374,7 @@ mod test { let function = test_file.functions[0].0.clone(); // execute function - let mut compiler = SingleFunctionCompiler::with_default_host_isa().unwrap(); + let compiler = SingleFunctionCompiler::with_default_host_isa().unwrap(); let compiled_function = compiler.compile(function).unwrap(); let returned = compiled_function.call(&[]); assert_eq!(returned, vec![DataValue::B(true)]) diff --git a/cranelift/filetests/src/test_run.rs b/cranelift/filetests/src/test_run.rs index 4abaa53e64..a817f14233 100644 --- a/cranelift/filetests/src/test_run.rs +++ b/cranelift/filetests/src/test_run.rs @@ -36,10 +36,24 @@ fn build_host_isa( let mut builder = cranelift_native::builder_with_options(infer_native_flags) .expect("Unable to build a TargetIsa for the current host"); + // Copy ISA Flags for value in isa_flags { builder.set(value.name, &value.value_string()).unwrap(); } + // We need to force disable stack probing, since we don't support it yet. + let flags = { + let mut flags_builder = settings::builder(); + + // Copy all flags + for flag in flags.iter() { + flags_builder.set(flag.name, &flag.value_string()).unwrap(); + } + + flags_builder.set("enable_probestack", "false").unwrap(); + settings::Flags::new(flags_builder) + }; + builder.finish(flags).unwrap() } @@ -118,20 +132,20 @@ impl SubTest for TestRun { return Ok(()); } - // We can't use the requested ISA directly since it does not contain info - // about the operating system / calling convention / etc.. - // - // Copy the requested ISA flags into the host ISA and use that. - let isa = build_host_isa(false, context.flags.clone(), requested_isa.isa_flags()); - let test_env = RuntestEnvironment::parse(&context.details.comments[..])?; - let mut compiler = SingleFunctionCompiler::new(isa); for comment in context.details.comments.iter() { if let Some(command) = parse_run_command(comment.text, &func.signature)? { trace!("Parsed run command: {}", command); - let compiled_fn = compiler.compile(func.clone().into_owned())?; + // We can't use the requested ISA directly since it does not contain info + // about the operating system / calling convention / etc.. + // + // Copy the requested ISA flags into the host ISA and use that. + let isa = build_host_isa(false, context.flags.clone(), requested_isa.isa_flags()); + + let compiled_fn = + SingleFunctionCompiler::new(isa).compile(func.clone().into_owned())?; command .run(|_, run_args| { test_env.validate_signature(&func)?; diff --git a/cranelift/fuzzgen/src/function_generator.rs b/cranelift/fuzzgen/src/function_generator.rs index 6fed1c4b2b..d8e6f78d64 100644 --- a/cranelift/fuzzgen/src/function_generator.rs +++ b/cranelift/fuzzgen/src/function_generator.rs @@ -710,7 +710,7 @@ where let sig = self.generate_signature()?; let mut fn_builder_ctx = FunctionBuilderContext::new(); - let mut func = Function::with_name_signature(ExternalName::user(0, 0), sig.clone()); + let mut func = Function::with_name_signature(ExternalName::user(0, 1), sig.clone()); let mut builder = FunctionBuilder::new(&mut func, &mut fn_builder_ctx); diff --git a/cranelift/jit/src/backend.rs b/cranelift/jit/src/backend.rs index 0948e1656a..75e8b2338e 100644 --- a/cranelift/jit/src/backend.rs +++ b/cranelift/jit/src/backend.rs @@ -886,7 +886,7 @@ fn lookup_with_dlsym(name: &str) -> Option<*const u8> { use windows_sys::Win32::Foundation::HINSTANCE; use windows_sys::Win32::System::LibraryLoader; - const MSVCRT_DLL: &[u8] = b"msvcrt.dll\0"; + const UCRTBASE: &[u8] = b"ucrtbase.dll\0"; let c_str = CString::new(name).unwrap(); let c_str_ptr = c_str.as_ptr(); @@ -896,7 +896,7 @@ fn lookup_with_dlsym(name: &str) -> Option<*const u8> { // try to find the searched symbol in the currently running executable ptr::null_mut(), // try to find the searched symbol in local c runtime - LibraryLoader::GetModuleHandleA(MSVCRT_DLL.as_ptr()) as RawHandle, + LibraryLoader::GetModuleHandleA(UCRTBASE.as_ptr()) as RawHandle, ]; for handle in &handles { diff --git a/cranelift/src/run.rs b/cranelift/src/run.rs index 089d382c90..c2a513423e 100644 --- a/cranelift/src/run.rs +++ b/cranelift/src/run.rs @@ -85,12 +85,11 @@ fn run_file_contents(file_contents: String) -> Result<()> { ..ParseOptions::default() }; let test_file = parse_test(&file_contents, options)?; - let isa = create_target_isa(&test_file.isa_spec)?; - let mut compiler = SingleFunctionCompiler::new(isa); for (func, Details { comments, .. }) in test_file.functions { for comment in comments { if let Some(command) = parse_run_command(comment.text, &func.signature)? { - let compiled_fn = compiler.compile(func.clone())?; + let isa = create_target_isa(&test_file.isa_spec)?; + let compiled_fn = SingleFunctionCompiler::new(isa).compile(func.clone())?; command .run(|_, args| Ok(compiled_fn.call(args))) .map_err(|s| anyhow::anyhow!("{}", s))?; diff --git a/fuzz/fuzz_targets/cranelift-fuzzgen.rs b/fuzz/fuzz_targets/cranelift-fuzzgen.rs index b4c3fe7d53..f4edd74421 100644 --- a/fuzz/fuzz_targets/cranelift-fuzzgen.rs +++ b/fuzz/fuzz_targets/cranelift-fuzzgen.rs @@ -68,7 +68,7 @@ fuzz_target!(|testcase: TestCase| { builder.set("enable_llvm_abi_extensions", "true").unwrap(); settings::Flags::new(builder) }; - let mut host_compiler = SingleFunctionCompiler::with_host_isa(flags).unwrap(); + let host_compiler = SingleFunctionCompiler::with_host_isa(flags).unwrap(); let compiled_fn = host_compiler.compile(testcase.func.clone()).unwrap(); for args in &testcase.inputs {