From d67b3b8302b902c57c68cc9f9c4fd8e3ce98e98c Mon Sep 17 00:00:00 2001 From: Afonso Bordado Date: Sat, 27 Aug 2022 10:47:18 +0100 Subject: [PATCH 1/3] cranelift: Implement `bnot` in interpreter --- .../filetests/filetests/runtests/bnot.clif | 45 +++++++++++++++++++ .../filetests/runtests/i128-bnot.clif | 11 +++++ cranelift/interpreter/src/value.rs | 2 +- 3 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 cranelift/filetests/filetests/runtests/bnot.clif create mode 100644 cranelift/filetests/filetests/runtests/i128-bnot.clif diff --git a/cranelift/filetests/filetests/runtests/bnot.clif b/cranelift/filetests/filetests/runtests/bnot.clif new file mode 100644 index 000000000000..e6e6a8674e21 --- /dev/null +++ b/cranelift/filetests/filetests/runtests/bnot.clif @@ -0,0 +1,45 @@ +test interpret +test run +target x86_64 +target aarch64 +target s390x + +function %bnot_b1(b1) -> b1 { +block0(v0: b1): + v1 = bnot.b1 v0 + return v1 +} +; run: %bnot_b1(false) == true +; run: %bnot_b1(true) == false + +function %bnot_b8(b8) -> b8 { +block0(v0: b8): + v1 = bnot.b8 v0 + return v1 +} +; run: %bnot_b8(false) == true +; run: %bnot_b8(true) == false + +function %bnot_b16(b16) -> b16 { +block0(v0: b16): + v1 = bnot.b16 v0 + return v1 +} +; run: %bnot_b16(false) == true +; run: %bnot_b16(true) == false + +function %bnot_b32(b32) -> b32 { +block0(v0: b32): + v1 = bnot.b32 v0 + return v1 +} +; run: %bnot_b32(false) == true +; run: %bnot_b32(true) == false + +function %bnot_b64(b64) -> b64 { +block0(v0: b64): + v1 = bnot.b64 v0 + return v1 +} +; run: %bnot_b64(false) == true +; run: %bnot_b64(true) == false diff --git a/cranelift/filetests/filetests/runtests/i128-bnot.clif b/cranelift/filetests/filetests/runtests/i128-bnot.clif new file mode 100644 index 000000000000..60e693ba9ae3 --- /dev/null +++ b/cranelift/filetests/filetests/runtests/i128-bnot.clif @@ -0,0 +1,11 @@ +test interpret +test run +target s390x + +function %bnot_b128(b128) -> b128 { +block0(v0: b128): + v1 = bnot.b128 v0 + return v1 +} +; run: %bnot_b128(false) == true +; run: %bnot_b128(true) == false diff --git a/cranelift/interpreter/src/value.rs b/cranelift/interpreter/src/value.rs index 01974b357f0b..0e0f738e4a5c 100644 --- a/cranelift/interpreter/src/value.rs +++ b/cranelift/interpreter/src/value.rs @@ -679,7 +679,7 @@ impl Value for DataValue { } fn not(self) -> ValueResult { - unary_match!(!(&self); [I8, I16, I32, I64]) + unary_match!(!(&self); [B, I8, I16, I32, I64]) } fn count_ones(self) -> ValueResult { From bcf90bcc367e1892b58f23efac0a5d9b4b051915 Mon Sep 17 00:00:00 2001 From: Afonso Bordado Date: Sat, 27 Aug 2022 10:48:04 +0100 Subject: [PATCH 2/3] cranelift: Register all functions in test file for interpreter --- .../filetests/filetests/runtests/call.clif | 1 + .../filetests/runtests/i128-call.clif | 1 + cranelift/filetests/src/test_interpret.rs | 109 ++++++++++++------ 3 files changed, 78 insertions(+), 33 deletions(-) diff --git a/cranelift/filetests/filetests/runtests/call.clif b/cranelift/filetests/filetests/runtests/call.clif index deb59423fe3c..a6a7202dcc85 100644 --- a/cranelift/filetests/filetests/runtests/call.clif +++ b/cranelift/filetests/filetests/runtests/call.clif @@ -1,3 +1,4 @@ +test interpret test run target x86_64 target aarch64 diff --git a/cranelift/filetests/filetests/runtests/i128-call.clif b/cranelift/filetests/filetests/runtests/i128-call.clif index 8170b24b3e5e..d4989a123549 100644 --- a/cranelift/filetests/filetests/runtests/i128-call.clif +++ b/cranelift/filetests/filetests/runtests/i128-call.clif @@ -1,3 +1,4 @@ +test interpret test run set enable_llvm_abi_extensions=true target x86_64 diff --git a/cranelift/filetests/src/test_interpret.rs b/cranelift/filetests/src/test_interpret.rs index 6232978948f0..14f191cfa9d1 100644 --- a/cranelift/filetests/src/test_interpret.rs +++ b/cranelift/filetests/src/test_interpret.rs @@ -3,16 +3,21 @@ //! The `interpret` test command interprets each function on the host machine //! using [RunCommand](cranelift_reader::RunCommand)s. +use crate::runone::FileUpdate; use crate::runtest_environment::RuntestEnvironment; -use crate::subtest::{Context, SubTest}; +use crate::subtest::SubTest; +use anyhow::{anyhow, Context}; use cranelift_codegen::data_value::DataValue; use cranelift_codegen::ir::types::I64; +use cranelift_codegen::ir::Function; +use cranelift_codegen::isa::TargetIsa; +use cranelift_codegen::settings::Flags; use cranelift_codegen::{self, ir}; use cranelift_interpreter::environment::FunctionStore; use cranelift_interpreter::interpreter::{HeapInit, Interpreter, InterpreterState}; use cranelift_interpreter::step::ControlFlow; -use cranelift_reader::{parse_run_command, TestCommand}; -use log::trace; +use cranelift_reader::{parse_run_command, Details, TestCommand, TestFile}; +use log::{info, trace}; use std::borrow::Cow; struct TestInterpret; @@ -38,43 +43,81 @@ impl SubTest for TestInterpret { false } - fn run(&self, func: Cow, context: &Context) -> anyhow::Result<()> { - let test_env = RuntestEnvironment::parse(&context.details.comments[..])?; - for comment in context.details.comments.iter() { - if let Some(command) = parse_run_command(comment.text, &func.signature)? { - trace!("Parsed run command: {}", command); + /// Runs the entire subtest for a given target, invokes [Self::run] for running + /// individual tests. + fn run_target<'a>( + &self, + testfile: &TestFile, + _: &mut FileUpdate, + _: &'a str, + _: &'a Flags, + _: Option<&'a dyn TargetIsa>, + ) -> anyhow::Result<()> { + // We can build the FunctionStore once and reuse it + let mut func_store = FunctionStore::default(); + for (func, _) in &testfile.functions { + func_store.add(func.name.to_string(), &func); + } - let mut env = FunctionStore::default(); - env.add(func.name.to_string(), &func); + for (func, details) in &testfile.functions { + info!("Test: {}({}) interpreter", self.name(), func.name); - command - .run(|func_name, run_args| { - test_env.validate_signature(&func)?; + let test_env = RuntestEnvironment::parse(&details.comments[..])?; + test_env.validate_signature(&func).map_err(|s| anyhow!(s))?; - let mut state = InterpreterState::default().with_function_store(env); + run_test(&test_env, &func_store, func, details).context(self.name())?; + } - let mut args = Vec::with_capacity(run_args.len()); - if test_env.is_active() { - let vmctx_addr = register_heaps(&mut state, &test_env); - args.push(vmctx_addr); - } - args.extend_from_slice(run_args); - - // Because we have stored function names with a leading %, we need to re-add it. - let func_name = &format!("%{}", func_name); - match Interpreter::new(state).call_by_name(func_name, &args) { - Ok(ControlFlow::Return(results)) => Ok(results.to_vec()), - Ok(_) => { - panic!("Unexpected returned control flow--this is likely a bug.") - } - Err(t) => Err(format!("unexpected trap: {:?}", t)), + Ok(()) + } + + fn run( + &self, + _func: Cow, + _context: &crate::subtest::Context, + ) -> anyhow::Result<()> { + unreachable!() + } +} + +fn run_test( + test_env: &RuntestEnvironment, + func_store: &FunctionStore, + func: &Function, + details: &Details, +) -> anyhow::Result<()> { + for comment in details.comments.iter() { + if let Some(command) = parse_run_command(comment.text, &func.signature)? { + trace!("Parsed run command: {}", command); + + command + .run(|func_name, run_args| { + // Rebuild the interpreter state on every run to ensure that we don't accidentally depend on + // some leftover state + let mut state = + InterpreterState::default().with_function_store(func_store.clone()); + + let mut args = Vec::with_capacity(run_args.len()); + if test_env.is_active() { + let vmctx_addr = register_heaps(&mut state, test_env); + args.push(vmctx_addr); + } + args.extend_from_slice(run_args); + + // Because we have stored function names with a leading %, we need to re-add it. + let func_name = &format!("%{}", func_name); + match Interpreter::new(state).call_by_name(func_name, &args) { + Ok(ControlFlow::Return(results)) => Ok(results.to_vec()), + Ok(e) => { + panic!("Unexpected returned control flow: {:?}", e) } - }) - .map_err(|e| anyhow::anyhow!("{}", e))?; - } + Err(t) => Err(format!("unexpected trap: {:?}", t)), + } + }) + .map_err(|e| anyhow::anyhow!("{}", e))?; } - Ok(()) } + Ok(()) } /// Build a VMContext struct with the layout described in docs/testing.md. From b0e50e2ee9eff9332790ead4ec9b4044919936b8 Mon Sep 17 00:00:00 2001 From: Afonso Bordado Date: Tue, 30 Aug 2022 09:23:21 +0100 Subject: [PATCH 3/3] cranelift: Relax signature checking for bools and vectors --- cranelift/interpreter/src/step.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/cranelift/interpreter/src/step.rs b/cranelift/interpreter/src/step.rs index 75ae1ce667fb..e5671e5512b1 100644 --- a/cranelift/interpreter/src/step.rs +++ b/cranelift/interpreter/src/step.rs @@ -22,7 +22,17 @@ fn validate_signature_params(sig: &[AbiParam], args: &[impl Value]) -> bool { args.iter() .map(|r| r.ty()) .zip(sig.iter().map(|r| r.value_type)) - .all(|(a, b)| a == b) + .all(|(a, b)| match (a, b) { + // For these two cases we don't have precise type information for `a`. + // We don't distinguish between different bool types, or different vector types + // The actual error is in `Value::ty` that returns default types for some values + // but we don't have enough information there either. + // + // Ideally the user has run the verifier and caught this properly... + (a, b) if a.is_bool() && b.is_bool() => true, + (a, b) if a.is_vector() && b.is_vector() => true, + (a, b) => a == b, + }) } /// Interpret a single Cranelift instruction. Note that program traps and interpreter errors are