Improve documentation of the filetest run command (#1645)

abrown · web-flow · commit d6796d0d23d8 · 2020-05-04T14:08:27.000-05:00
* 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.
diff --git 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
@@ -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
@@ -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
@@ -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<DataValue>`.
 fn write_data_value_list(f: &mut Formatter<'_>, list: &[DataValue]) -> fmt::Result {
     match list.len() {
