From d6796d0d23d8aa06d577c408873596b326fc826e Mon Sep 17 00:00:00 2001 From: Andrew Brown Date: Mon, 4 May 2020 12:08:27 -0700 Subject: [PATCH] Improve documentation of the filetest `run` command (#1645) * Improve output display of RunCommand The previous use of Debug for displaying `print` and `run` results was less than clear. * Avoid checking the types of vectors during trampoline construction Because DataValue only understands `V128` vectors, we avoid type-checking vector values when constructing the trampoline arguments. * Improve the documentation of the filetest `run` command Adds an up-to-date example of how to use the `run` and `print` directives and includes an actual use of the new directives in a SIMD arithmetic filetest. --- cranelift/docs/testing.md | 39 ++++++++++++++---- .../isa/x86/simd-arithmetic-run.clif | 30 ++++---------- cranelift/filetests/src/function_runner.rs | 5 +-- cranelift/reader/src/run_command.rs | 41 ++++++++++++++----- 4 files changed, 72 insertions(+), 43 deletions(-) diff --git a/cranelift/docs/testing.md b/cranelift/docs/testing.md index 92dc99f33a..1918325742 100644 --- a/cranelift/docs/testing.md +++ b/cranelift/docs/testing.md @@ -320,13 +320,21 @@ Cranelift IR right before binary machine code emission. Compile and execute a function. -Add a `; run` directive after each function that should be executed. These -functions must have the signature `() -> bNN` where `bNN` is some sort of -boolean, e.g. `b1` or `b32`. A `true` value is interpreted as a successful -test execution, whereas a `false` value is interpreted as a failed test. +This test command allows several directives: + - to print the result of running a function to stdout, add a `print` + directive and call the preceding function with arguments (see `%foo` in + the example below); remember to enable `--nocapture` if running these + tests through Cargo + - to check the result of a function, add a `run` directive and call the + preceding function with a comparison (`==` or `!=`) (see `%bar` below) + - for backwards compatibility, to check the result of a function with a + `() -> b*` signature, only the `run` directive is required, with no + invocation or comparison (see `%baz` below); a `true` value is + interpreted as a successful test execution, whereas a `false` value is + interpreted as a failed test. Currently a `target` is required but is only used to indicate whether the host -platform can run the test, and currently only the architecture is filtered. The +platform can run the test and currently only the architecture is filtered. The host platform's native target will be used to actually compile the test. Example: @@ -335,8 +343,25 @@ Example: test run target x86_64 - function %trivial_test() -> b1 { - ebb0: + ; how to print the results of a function + function %foo() -> i32 { + block0: + v0 = iconst.i32 42 + return v0 + } + ; print: %foo() + + ; how to check the results of a function + function %bar(i32) -> i32 { + block0(v0:i32): + v1 = iadd_imm v0, 1 + return v1 + } + ; run: %bar(1) == 2 + + ; legacy method of checking the results of a function + function %baz() -> b1 { + block0: v0 = bconst.b1 true return v0 } diff --git a/cranelift/filetests/filetests/isa/x86/simd-arithmetic-run.clif b/cranelift/filetests/filetests/isa/x86/simd-arithmetic-run.clif index 8f2b46f19e..30ce4f7103 100644 --- a/cranelift/filetests/filetests/isa/x86/simd-arithmetic-run.clif +++ b/cranelift/filetests/filetests/isa/x86/simd-arithmetic-run.clif @@ -2,37 +2,21 @@ test run set enable_simd target x86_64 skylake -function %iadd_i32x4() -> b1 { -block0: - v0 = vconst.i32x4 [1 1 1 1] - v1 = vconst.i32x4 [1 2 3 4] +function %iadd_i32x4(i32x4, i32x4) -> i32x4 { +block0(v0:i32x4, v1:i32x4): v2 = iadd v0, v1 - - v3 = extractlane v2, 0 - v4 = icmp_imm eq v3, 2 - - v5 = extractlane v2, 3 - v6 = icmp_imm eq v5, 5 - ; TODO replace extractlanes with vector comparison - - v7 = band v4, v6 - return v7 + return v2 } -; run +; run: %iadd_i32x4([1 1 1 1], [1 2 3 4]) == [2 3 4 5] -function %iadd_i8x16_with_overflow() -> b1 { +function %iadd_i8x16_with_overflow() -> i8x16 { block0: v0 = vconst.i8x16 [255 255 255 255 255 255 255 255 255 255 255 255 255 255 255 255] v1 = vconst.i8x16 [2 2 2 2 2 2 2 2 2 2 2 2 2 2 2 2] v2 = iadd v0, v1 - - v3 = extractlane v2, 0 - v4 = icmp_imm eq v3, 1 - ; TODO replace extractlane with vector comparison - - return v4 + return v2 } -; run +; run: %iadd_i8x16_with_overflow() == [1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1] function %isub_i32x4_rex() -> b1 { block0: diff --git a/cranelift/filetests/src/function_runner.rs b/cranelift/filetests/src/function_runner.rs index e0a45ddb43..69ad26029b 100644 --- a/cranelift/filetests/src/function_runner.rs +++ b/cranelift/filetests/src/function_runner.rs @@ -189,9 +189,8 @@ impl UnboxedValues { // Store the argument values into `values_vec`. for ((arg, slot), param) in arguments.iter().zip(&mut values_vec).zip(&signature.params) { - assert_eq!( - arg.ty(), - param.value_type, + assert!( + arg.ty() == param.value_type || arg.is_vector(), "argument type mismatch: {} != {}", arg.ty(), param.value_type diff --git a/cranelift/reader/src/run_command.rs b/cranelift/reader/src/run_command.rs index 7c7e54eaf1..27bd81dec6 100644 --- a/cranelift/reader/src/run_command.rs +++ b/cranelift/reader/src/run_command.rs @@ -34,7 +34,7 @@ impl RunCommand { match self { RunCommand::Print(invoke) => { let actual = invoke_fn(&invoke.args); - println!("{:?} -> {:?}", invoke, actual) + println!("{} -> {}", invoke, DisplayDataValues(&actual)) } RunCommand::Run(invoke, compare, expected) => { let actual = invoke_fn(&invoke.args); @@ -43,7 +43,8 @@ impl RunCommand { Comparison::NotEquals => *expected != actual, }; if !matched { - return Err(format!("Failed test: {:?}, actual: {:?}", self, actual)); + let actual = DisplayDataValues(&actual); + return Err(format!("Failed test: {}, actual: {}", self, actual)); } } } @@ -56,14 +57,8 @@ impl Display for RunCommand { match self { RunCommand::Print(invocation) => write!(f, "print: {}", invocation), RunCommand::Run(invocation, comparison, expected) => { - write!(f, "run: {} {} ", invocation, comparison)?; - if expected.len() == 1 { - write!(f, "{}", expected[0]) - } else { - write!(f, "[")?; - write_data_value_list(f, expected)?; - write!(f, "]") - } + let expected = DisplayDataValues(expected); + write!(f, "run: {} {} {}", invocation, comparison, expected) } } } @@ -125,6 +120,14 @@ impl DataValue { DataValue::V128(_) => ir::types::I8X16, } } + + /// Return true if the value is a vector (i.e. `DataValue::V128`). + pub fn is_vector(&self) -> bool { + match self { + DataValue::V128(_) => true, + _ => false, + } + } } /// Helper for creating [From] implementations for [DataValue] @@ -163,6 +166,24 @@ impl Display for DataValue { } } +/// Helper structure for printing bracket-enclosed vectors of [DataValue]s. +/// - for empty vectors, display `[]` +/// - for single item vectors, display `42`, e.g. +/// - for multiple item vectors, display `[42, 43, 44]`, e.g. +struct DisplayDataValues<'a>(&'a [DataValue]); + +impl<'a> Display for DisplayDataValues<'a> { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + if self.0.len() == 1 { + write!(f, "{}", self.0[0]) + } else { + write!(f, "[")?; + write_data_value_list(f, &self.0)?; + write!(f, "]") + } + } +} + /// Helper function for displaying `Vec`. fn write_data_value_list(f: &mut Formatter<'_>, list: &[DataValue]) -> fmt::Result { match list.len() {