Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add precise_output argument to test optimize. #6111

Merged
merged 2 commits into from
Mar 28, 2023

Conversation

Kmeakin
Copy link
Contributor

@Kmeakin Kmeakin commented Mar 28, 2023

Allow test optimize tests to take the precise-output argument, and to be updated by CRANELIFT_TEST_BLESS, similar to test compile

Also allow optimise tests to be updated by `CRANELIFT_TEST_BLESS=1`
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Mar 28, 2023
@jameysharp
Copy link
Contributor

This is neat! It's clearly handy to be able to regenerate test expectations like this.

I don't know for sure that precise output tests are a good idea for optimizations though. Writing assertions about only the specific aspects we're trying to test makes the tests more resilient to unrelated changes in the optimizer.

I'm curious if @cfallin or @fitzgen have opinions on this.

@Kmeakin
Copy link
Contributor Author

Kmeakin commented Mar 28, 2023

I don't know for sure that precise output tests are a good idea for optimizations though. Writing assertions about only the specific aspects we're trying to test makes the tests more resilient to unrelated changes in the optimizer.

Could we do a script to automatically insert the correct filecheck commands? LLVM has something similar: https://github.com/llvm/llvm-project/blob/main/llvm/utils/update_any_test_checks.py

@cfallin
Copy link
Member

cfallin commented Mar 28, 2023

I don't know for sure that precise output tests are a good idea for optimizations though. Writing assertions about only the specific aspects we're trying to test makes the tests more resilient to unrelated changes in the optimizer.

I'm curious if @cfallin or @fitzgen have opinions on this.

Yes, I think we want to retain this property: many of the tests are checking just "this one value becomes X" rather than freezing the entire output. There is still some unnecessary capture of implementation details (e.g. the value number in the output exposes how many rewrites we went through) but less coupling is better here.

It would be nice to try to find ways to automate updates though!

@fitzgen
Copy link
Member

fitzgen commented Mar 28, 2023

Yeah for tests that are asserting one particular peephole applied, it is nice to not have the full precise output. There are certainly other tests that are written in a less targeted way, and could be migrated to precise output tests on a case-by-case basis. Because of this, it would be nice to support, even if we don't blanket enable it for every test optimize test.

Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that Nick thinks we'll find uses for this, let's go ahead and merge it. I've reviewed it and it's good, but I do have one thing I want changed before merge:

Let's share check_precise_output and update_test between all subtest modes that have a precise-output option. I think it makes sense to put these functions in cranelift/filetests/src/subtest.rs since they only rely on Context. Optionally, they could be methods on Context.

If I'm not mistaken, this copy of update_test is identical to the one in test_compile.rs, and this copy of check_precise_output is almost identical except for requiring the caller to provide a slice of strings. The latter is easily unified by moving the definition of actual to run in test_compile.rs.

Looking at this diff, there are several things I'd like to change in these functions, but they're all things that were pre-existing in test_compile.rs, so let's not change them in this PR.

cranelift/filetests/src/test_optimize.rs Outdated Show resolved Hide resolved
@Kmeakin Kmeakin force-pushed the test-optimize-bless branch from b8ab7a9 to 585cb1f Compare March 28, 2023 20:49
Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you!

@jameysharp jameysharp added this pull request to the merge queue Mar 28, 2023
Merged via the queue into bytecodealliance:main with commit 97d9f77 Mar 28, 2023
@Kmeakin Kmeakin deleted the test-optimize-bless branch March 28, 2023 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants