Add precise_output argument to test optimize. (#6111)

* Add `precise_output` argument to `test optimise`.

Also allow optimise tests to be updated by `CRANELIFT_TEST_BLESS=1`

* Move `check_precise_output` and `update_test` to `subtest`

cranelift/filetests/src/subtest.rs

@@ -2,13 +2,16 @@
 
 use crate::runone::FileUpdate;
 use anyhow::Context as _;
+use anyhow::{bail, Result};
 use cranelift_codegen::ir::Function;
 use cranelift_codegen::isa::TargetIsa;
 use cranelift_codegen::settings::{Flags, FlagsOrIsa};
 use cranelift_reader::{Comment, Details, TestFile};
 use filecheck::{Checker, CheckerBuilder, NO_VARIABLES};
 use log::info;
+use similar::TextDiff;
 use std::borrow::Cow;
+use std::env;
 
 /// Context for running a test on a single function.
 pub struct Context<'a> {
@@ -149,3 +152,74 @@ pub fn build_filechecker(context: &Context) -> anyhow::Result<Checker> {
     }
     Ok(builder.finish())
 }
+
+pub fn check_precise_output(actual: &[&str], context: &Context) -> Result<()> {
+    // Use the comments after the function to build the test expectation.
+    let expected = context
+        .details
+        .comments
+        .iter()
+        .filter(|c| !c.text.starts_with(";;"))
+        .map(|c| c.text.strip_prefix("; ").unwrap_or(c.text))
+        .collect::<Vec<_>>();
+
+    // If the expectation matches what we got, then there's nothing to do.
+    if actual == expected {
+        return Ok(());
+    }
+
+    // If we're supposed to automatically update the test, then do so here.
+    if env::var("CRANELIFT_TEST_BLESS").unwrap_or(String::new()) == "1" {
+        return update_test(&actual, context);
+    }
+
+    // Otherwise this test has failed, and we can print out as such.
+    bail!(
+        "compilation of function on line {} does not match\n\
+         the text expectation\n\
+         \n\
+         {}\n\
+         \n\
+         This test assertion can be automatically updated by setting the\n\
+         CRANELIFT_TEST_BLESS=1 environment variable when running this test.
+         ",
+        context.details.location.line_number,
+        TextDiff::from_slices(&expected, &actual)
+            .unified_diff()
+            .header("expected", "actual")
+    )
+}
+
+fn update_test(output: &[&str], context: &Context) -> Result<()> {
+    context
+        .file_update
+        .update_at(&context.details.location, |new_test, old_test| {
+            // blank newline after the function
+            new_test.push_str("\n");
+
+            // Splice in the test output
+            for output in output {
+                new_test.push_str("; ");
+                new_test.push_str(output);
+                new_test.push_str("\n");
+            }
+
+            // blank newline after test assertion
+            new_test.push_str("\n");
+
+            // Drop all remaining commented lines (presumably the old test expectation),
+            // but after we hit a real line then we push all remaining lines.
+            let mut in_next_function = false;
+            for line in old_test {
+                if !in_next_function
+                    && (line.trim().is_empty()
+                        || (line.starts_with(";") && !line.starts_with(";;")))
+                {
+                    continue;
+                }
+                in_next_function = true;
+                new_test.push_str(line);
+                new_test.push_str("\n");
+            }
+        })
+}

cranelift/filetests/src/test_compile.rs

@@ -2,17 +2,12 @@
 //!
 //! The `compile` test command runs each function through the full code generator pipeline
 
-use crate::subtest::{run_filecheck, Context, SubTest};
+use crate::subtest::{check_precise_output, run_filecheck, Context, SubTest};
-use anyhow::{bail, Result};
+use anyhow::Result;
 use cranelift_codegen::ir;
-use cranelift_codegen::ir::function::FunctionParameters;
-use cranelift_codegen::isa;
-use cranelift_codegen::CompiledCode;
 use cranelift_reader::{TestCommand, TestOption};
 use log::info;
-use similar::TextDiff;
 use std::borrow::Cow;
-use std::env;
 
 struct TestCompile {
     /// Flag indicating that the text expectation, comments after the function,
@@ -67,97 +62,20 @@ impl SubTest for TestCompile {
         info!("Generated {} bytes of code:\n{}", total_size, vcode);
 
         if self.precise_output {
-            check_precise_output(isa, &params, &compiled_code, context)
+            let cs = isa
+                .to_capstone()
+                .map_err(|e| anyhow::format_err!("{}", e))?;
+            let dis = compiled_code.disassemble(Some(&params), &cs)?;
+
+            let actual = Vec::from_iter(
+                std::iter::once("VCode:")
+                    .chain(compiled_code.vcode.as_ref().unwrap().lines())
+                    .chain(["", "Disassembled:"])
+                    .chain(dis.lines()),
+            );
+            check_precise_output(&actual, context)
         } else {
             run_filecheck(&vcode, context)
         }
     }
 }
-
-fn check_precise_output(
-    isa: &dyn isa::TargetIsa,
-    params: &FunctionParameters,
-    compiled_code: &CompiledCode,
-    context: &Context,
-) -> Result<()> {
-    let cs = isa
-        .to_capstone()
-        .map_err(|e| anyhow::format_err!("{}", e))?;
-    let dis = compiled_code.disassemble(Some(params), &cs)?;
-
-    let actual = Vec::from_iter(
-        std::iter::once("VCode:")
-            .chain(compiled_code.vcode.as_ref().unwrap().lines())
-            .chain(["", "Disassembled:"])
-            .chain(dis.lines()),
-    );
-
-    // Use the comments after the function to build the test expectation.
-    let expected = context
-        .details
-        .comments
-        .iter()
-        .filter(|c| !c.text.starts_with(";;"))
-        .map(|c| c.text.strip_prefix("; ").unwrap_or(c.text))
-        .collect::<Vec<_>>();
-
-    // If the expectation matches what we got, then there's nothing to do.
-    if actual == expected {
-        return Ok(());
-    }
-
-    // If we're supposed to automatically update the test, then do so here.
-    if env::var("CRANELIFT_TEST_BLESS").unwrap_or(String::new()) == "1" {
-        return update_test(&actual, context);
-    }
-
-    // Otherwise this test has failed, and we can print out as such.
-    bail!(
-        "compilation of function on line {} does not match\n\
-         the text expectation\n\
-         \n\
-         {}\n\
-         \n\
-         This test assertion can be automatically updated by setting the\n\
-         CRANELIFT_TEST_BLESS=1 environment variable when running this test.
-         ",
-        context.details.location.line_number,
-        TextDiff::from_slices(&expected, &actual)
-            .unified_diff()
-            .header("expected", "actual")
-    )
-}
-
-fn update_test(output: &[&str], context: &Context) -> Result<()> {
-    context
-        .file_update
-        .update_at(&context.details.location, |new_test, old_test| {
-            // blank newline after the function
-            new_test.push_str("\n");
-
-            // Splice in the test output
-            for output in output {
-                new_test.push_str("; ");
-                new_test.push_str(output);
-                new_test.push_str("\n");
-            }
-
-            // blank newline after test assertion
-            new_test.push_str("\n");
-
-            // Drop all remaining commented lines (presumably the old test expectation),
-            // but after we hit a real line then we push all remaining lines.
-            let mut in_next_function = false;
-            for line in old_test {
-                if !in_next_function
-                    && (line.trim().is_empty()
-                        || (line.starts_with(";") && !line.starts_with(";;")))
-                {
-                    continue;
-                }
-                in_next_function = true;
-                new_test.push_str(line);
-                new_test.push_str("\n");
-            }
-        })
-}

cranelift/filetests/src/test_optimize.rs

@@ -7,17 +7,32 @@
 //! Some legalization may be ISA-specific, so this requires an ISA
 //! (for now).
 
-use crate::subtest::{run_filecheck, Context, SubTest};
+use crate::subtest::{check_precise_output, run_filecheck, Context, SubTest};
 use anyhow::Result;
 use cranelift_codegen::ir;
-use cranelift_reader::TestCommand;
+use cranelift_reader::{TestCommand, TestOption};
 use std::borrow::Cow;
 
-struct TestOptimize;
+struct TestOptimize {
+    /// Flag indicating that the text expectation, comments after the function,
+    /// must be a precise 100% match on the compiled output of the function.
+    /// This test assertion is also automatically-update-able to allow tweaking
+    /// the code generator and easily updating all affected tests.
+    precise_output: bool,
+}
 
 pub fn subtest(parsed: &TestCommand) -> Result<Box<dyn SubTest>> {
     assert_eq!(parsed.command, "optimize");
-    Ok(Box::new(TestOptimize))
+    let mut test = TestOptimize {
+        precise_output: false,
+    };
+    for option in parsed.options.iter() {
+        match option {
+            TestOption::Flag("precise-output") => test.precise_output = true,
+            _ => anyhow::bail!("unknown option on {}", parsed),
+        }
+    }
+    Ok(Box::new(test))
 }
 
 impl SubTest for TestOptimize {
@@ -42,6 +57,12 @@ impl SubTest for TestOptimize {
             .map_err(|e| crate::pretty_anyhow_error(&comp_ctx.func, e))?;
 
         let clif = format!("{:?}", comp_ctx.func);
-        run_filecheck(&clif, context)
+
+        if self.precise_output {
+            let actual: Vec<_> = clif.lines().collect();
+            check_precise_output(&actual, context)
+        } else {
+            run_filecheck(&clif, context)
+        }
     }
 }