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 3d6bfc50d136..aeee11bec480 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 47a2b49c59da..1a2415766a8b 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 8031c76d573e..89f8abcf8f2c 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 5a15c8be8c89..cbb67aad4f81 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 90e3d566e834..c96c2a6e4f48 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 dd265a18e291..cd2397e9adba 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 99410b604319..0de97280fb63 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 62efa830248e..ba1ca323e5fe 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 4abaa53e64e6..a817f142336e 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 6fed1c4b2b4e..d8e6f78d64a6 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 0948e1656aac..75e8b2338ecd 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 089d382c901b..c2a513423ea7 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 b4c3fe7d53df..f4edd7442100 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 {