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 FileCheck annotations to MIR-opt tests #116971

Open
cjgillot opened this issue Oct 20, 2023 · 12 comments
Open

Add FileCheck annotations to MIR-opt tests #116971

cjgillot opened this issue Oct 20, 2023 · 12 comments
Labels
A-mir-opt Area: MIR optimizations A-testsuite Area: The testsuite used to check the correctness of rustc E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. E-tedious Call for participation: An issue involves lots of work and is better handled as many small tasks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cjgillot
Copy link
Contributor

The MIR-opt testsuite (tests/mir-opt) supports checking the MIR output using LLVM FileCheck tool. The purpose of this check is to write in the .rs file what we expect to see, similar to the annotations we have for ui tests. See individual commits in #116810 for a few examples, and tests/mir-opt/README.md for tips in dealing with functions, locals and basic blocks.

Most testcases disable this using a // skip-filecheck comment. They should not.
We need to go through all the test cases in the MIR-opt suite to write these annotations.
Exception: the tests/mir-opt/pre-codegen does not need these directives, as its purpose is to track the quality of the MIR we emit.

Steps, for each test case:

  • figure out what is being tested (that's sometimes not directly obvious, may need git archeology);
  • write the FileCheck annotations based on the emitted MIR (see the MIR dumps to help).

In some cases, it may be interesting to strengthen the test cases to better verify what they meant to verify. This can be done by adding a // unit-test: SomePassName directive at the top, or changing compiler flags.

The point is to add check directives for what we want to check. Skipping storage statements, basic-block numbers, useless assignments is encouraged.

For volunteers: please post a comment with the files you want to work with. Please do not claim the whole issue, there is plenty to do. Please r? cjgillot in the PR so I can keep track of them.

@cjgillot cjgillot added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. E-help-wanted Call for participation: Help is requested to fix this issue. labels Oct 20, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 20, 2023
@Noratrieb Noratrieb added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-mir-opt Area: MIR optimizations and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 20, 2023
@rmehri01
Copy link
Contributor

rmehri01 commented Oct 20, 2023

I'll try working on tests/mir-opt/inline/ :)

@DavidCoy77
Copy link

Hi! I'd love to get started contributing to Rust, but I'd need a little help getting started since this would be my first ever GitHub contribution. You've included the tags Easy, and Help-Wanted, so since there are plenty of files to work on besides the one @rmehri01 claimed, if you could help me out I'm eager to get to work.

@rmehri01
Copy link
Contributor

rmehri01 commented Oct 24, 2023

Hey there, I can offer a bit of insight but maybe someone else can add more details :)

For getting started, check out CONTRIBUTING.md if you haven't already, the rustc dev guide is really comprehensive and helpful!

For this issue specifically, I basically went to each file in the above directory and went through the GitHub history (related issues and PRs, especially the initial PR that created the test file) to try to determine what is trying to be tested. The diff files for each test are also really helpful for this since they show what changes the test actually made. Along with the file name and comments, you can usually get an idea of what to test. Then you can use the LLVM FileCheck syntax to check the expected result.

Feel free to reach out if you're stuck, I'll try my best to help.

@DavidCoy77
Copy link

DavidCoy77 commented Oct 24, 2023

I'm still a beginner so I definitely appreciate the willingness to help! Let me know if I have a good handle on the issue: the tests/mir-opt/ directory contains code for various tests you can run from the command line to test MIR optimizations. After some minor "git-archaeology" it seems the pattern is the // skip-filecheck needs to be added to the top of the .rs files, and the filecheck annotation code needs to be added to the corresponding .diff file.

You mention running the FileCheck test, and getting an idea of what to test, but how would I go about that? Additionally, I don't quite understand the code in the .diff files. As a beginner I really appreciate your time and effort!

@rmehri01
Copy link
Contributor

Not quite, the // skip-filecheck is actually there right now to disable running FileCheck:

Most testcases disable this using a // skip-filecheck comment. They should not.

And we want to add the FileCheck annotations to the source .rs code, the diffs are just helpful for seeing what changes due to the MIR optimization.

You can check out the commits in the PR mentioned in the issue or in my PR for some examples. You want to remove that // skip-filecheck line so that FileCheck does run on the file. As for running FileCheck, this is already done in the previous PR mentioned in this issue, so running the tests with ./x test tests/mir-opt --stage 1 for example will run FileCheck automatically. For the code in the diff files, https://rustc-dev-guide.rust-lang.org/mir/index.html (the linked blog post is quite helpful) and https://rustc-dev-guide.rust-lang.org/mir/optimizations.html may help.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 24, 2023
…tests, r=cjgillot

Add FileCheck annotations to MIR-opt inlining tests

Part of rust-lang#116971, adds FileCheck annotations to MIR-opt tests in `tests/mir-opt/inline`.

I left out a few (such as `inline_cycle`) where it mentioned that the particular outcome of inlining isn't important, just that the inliner doesn't get stuck in an infinite loop.

r? cjgillot
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 28, 2023
…tests, r=cjgillot

Add FileCheck annotations to MIR-opt inlining tests

Part of rust-lang#116971, adds FileCheck annotations to MIR-opt tests in `tests/mir-opt/inline`.

I left out a few (such as `inline_cycle`) where it mentioned that the particular outcome of inlining isn't important, just that the inliner doesn't get stuck in an infinite loop.

r? cjgillot
@tjausm
Copy link

tjausm commented Oct 31, 2023

I'll start on the tests/mir-opt/const_prop/ folder

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 1, 2023
…tests, r=cjgillot

Add FileCheck annotations to MIR-opt inlining tests

Part of rust-lang#116971, adds FileCheck annotations to MIR-opt tests in `tests/mir-opt/inline`.

I left out a few (such as `inline_cycle`) where it mentioned that the particular outcome of inlining isn't important, just that the inliner doesn't get stuck in an infinite loop.

r? cjgillot
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 1, 2023
…sts, r=cjgillot

Add FileCheck annotations to MIR-opt inlining tests

Part of rust-lang#116971, adds FileCheck annotations to MIR-opt tests in `tests/mir-opt/inline`.

I left out a few (such as `inline_cycle`) where it mentioned that the particular outcome of inlining isn't important, just that the inliner doesn't get stuck in an infinite loop.

r? cjgillot
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 10, 2023
…k-Simulacrum

Add FileCheck annotations to const_prop tests

Unblocks rust-lang#116012
Advances rust-lang#116971
bors added a commit to rust-lang/miri that referenced this issue Dec 10, 2023
@sasurau4
Copy link
Contributor

I'll start on the tests/mir-opt/copy-prop/ folder

@Tyrubias
Copy link
Contributor

I'll work on the tests/mir-opt/dest-prop/ folder.

@sfzhu93
Copy link
Contributor

sfzhu93 commented Jan 7, 2024

I've started on directory tests/mir-opt/dataflow-const-prop.

Nadrieril added a commit to Nadrieril/rust that referenced this issue Jan 27, 2024
Add FileCheck annotations to dataflow-const-prop tests

part of rust-lang#116971.

A few shadowing variable names are changed, so that it is easier to match the variable names in MIR using FileCheck syntax.

Also, there's a FIXME in [enum.rs](https://github.com/rust-lang/rust/pull/119759/files#diff-7621f55327838e489a95ac99ae1e6126b37c57aff582594e6bee9d7e7e56fc58) because the MIR looks suspicious to me. It has been explained in the comments.

r? cjgillot
Nadrieril added a commit to Nadrieril/rust that referenced this issue Jan 27, 2024
Add FileCheck annotations to dataflow-const-prop tests

part of rust-lang#116971.

A few shadowing variable names are changed, so that it is easier to match the variable names in MIR using FileCheck syntax.

Also, there's a FIXME in [enum.rs](https://github.com/rust-lang/rust/pull/119759/files#diff-7621f55327838e489a95ac99ae1e6126b37c57aff582594e6bee9d7e7e56fc58) because the MIR looks suspicious to me. It has been explained in the comments.

r? cjgillot
@sfzhu93
Copy link
Contributor

sfzhu93 commented Jan 27, 2024

Just some experience to share: we should test against 32 bit and 64 bit platforms, as well as platforms with different unwinding. wasm32-unknown-unknown is helpful because it differs from x86_64-unknown-linux-gnu in both bits and unwinding.

@JarlEvanson
Copy link
Contributor

I'll work on the tests/mir-opt/sroa folder.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 1, 2024
Add FileCheck annotations to dataflow-const-prop tests

part of rust-lang#116971.

A few shadowing variable names are changed, so that it is easier to match the variable names in MIR using FileCheck syntax.

Also, there's a FIXME in [enum.rs](https://github.com/rust-lang/rust/pull/119759/files#diff-7621f55327838e489a95ac99ae1e6126b37c57aff582594e6bee9d7e7e56fc58) because the MIR looks suspicious to me. It has been explained in the comments.

r? cjgillot
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 1, 2024
Add FileCheck annotations to dataflow-const-prop tests

part of rust-lang#116971.

A few shadowing variable names are changed, so that it is easier to match the variable names in MIR using FileCheck syntax.

Also, there's a FIXME in [enum.rs](https://github.com/rust-lang/rust/pull/119759/files#diff-7621f55327838e489a95ac99ae1e6126b37c57aff582594e6bee9d7e7e56fc58) because the MIR looks suspicious to me. It has been explained in the comments.

r? cjgillot
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 4, 2024
Add FileCheck annotations to dataflow-const-prop tests

part of rust-lang#116971.

A few shadowing variable names are changed, so that it is easier to match the variable names in MIR using FileCheck syntax.

Also, there's a FIXME in [enum.rs](https://github.com/rust-lang/rust/pull/119759/files#diff-7621f55327838e489a95ac99ae1e6126b37c57aff582594e6bee9d7e7e56fc58) because the MIR looks suspicious to me. It has been explained in the comments.

r? cjgillot
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 5, 2024
Rollup merge of rust-lang#119759 - sfzhu93:master, r=cjgillot

Add FileCheck annotations to dataflow-const-prop tests

part of rust-lang#116971.

A few shadowing variable names are changed, so that it is easier to match the variable names in MIR using FileCheck syntax.

Also, there's a FIXME in [enum.rs](https://github.com/rust-lang/rust/pull/119759/files#diff-7621f55327838e489a95ac99ae1e6126b37c57aff582594e6bee9d7e7e56fc58) because the MIR looks suspicious to me. It has been explained in the comments.

r? cjgillot
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 7, 2024
…illot

 Add FileCheck annotations to MIR-opt SROA tests

Part of rust-lang#116971, adds FileCheck annotations to SROA MIR-opt tests in `tests/mir-opt/sroa` and a few uncategorized files.

r? cjgillot
Nadrieril added a commit to Nadrieril/rust that referenced this issue Feb 7, 2024
…illot

 Add FileCheck annotations to MIR-opt SROA tests

Part of rust-lang#116971, adds FileCheck annotations to SROA MIR-opt tests in `tests/mir-opt/sroa` and a few uncategorized files.

r? cjgillot
Nadrieril added a commit to Nadrieril/rust that referenced this issue Feb 7, 2024
…illot

 Add FileCheck annotations to MIR-opt SROA tests

Part of rust-lang#116971, adds FileCheck annotations to SROA MIR-opt tests in `tests/mir-opt/sroa` and a few uncategorized files.

r? cjgillot
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 7, 2024
Rollup merge of rust-lang#120455 - JarlEvanson:sroa-miri-tests, r=cjgillot

 Add FileCheck annotations to MIR-opt SROA tests

Part of rust-lang#116971, adds FileCheck annotations to SROA MIR-opt tests in `tests/mir-opt/sroa` and a few uncategorized files.

r? cjgillot
@CastilloDel
Copy link
Contributor

I'll work on the tests/mir-opt/dest-prop folder

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 11, 2024
…ck, r=oli-obk

Add FileCheck annotations to MIR-opt unnamed-fields tests

Part of rust-lang#116971
Adds filecheck annotations to unnamed-fields mir-opt tests in `tests/mir-opt/unnamed-fields`
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Mar 11, 2024
…ck, r=oli-obk

Add FileCheck annotations to MIR-opt unnamed-fields tests

Part of rust-lang#116971
Adds filecheck annotations to unnamed-fields mir-opt tests in `tests/mir-opt/unnamed-fields`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 12, 2024
…ck, r=oli-obk

Add FileCheck annotations to MIR-opt unnamed-fields tests

Part of rust-lang#116971
Adds filecheck annotations to unnamed-fields mir-opt tests in `tests/mir-opt/unnamed-fields`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 12, 2024
Rollup merge of rust-lang#121865 - Kirandevraj:unnamed-fields-filecheck, r=oli-obk

Add FileCheck annotations to MIR-opt unnamed-fields tests

Part of rust-lang#116971
Adds filecheck annotations to unnamed-fields mir-opt tests in `tests/mir-opt/unnamed-fields`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Jul 13, 2024
Add FileCheck annotations to mir-opt/dest-prop tests

Part of rust-lang#116971, adds FileCheck annotations to MIR-opt tests in tests/mir-opt/dest-prop.

I would like some feedback. Also, I don't know how to approach `union.rs`.  I couldn't figure out what it is testing.

r? cjgillot
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Jul 13, 2024
Add FileCheck annotations to mir-opt/dest-prop tests

Part of rust-lang#116971, adds FileCheck annotations to MIR-opt tests in tests/mir-opt/dest-prop.

I would like some feedback. Also, I don't know how to approach `union.rs`.  I couldn't figure out what it is testing.

r? cjgillot
jhpratt added a commit to jhpratt/rust that referenced this issue Jul 13, 2024
Add FileCheck annotations to mir-opt/dest-prop tests

Part of rust-lang#116971, adds FileCheck annotations to MIR-opt tests in tests/mir-opt/dest-prop.

I would like some feedback. Also, I don't know how to approach `union.rs`.  I couldn't figure out what it is testing.

r? cjgillot
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Jul 14, 2024
Add FileCheck annotations to mir-opt/dest-prop tests

Part of rust-lang#116971, adds FileCheck annotations to MIR-opt tests in tests/mir-opt/dest-prop.

I would like some feedback. Also, I don't know how to approach `union.rs`.  I couldn't figure out what it is testing.

r? cjgillot
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 14, 2024
Rollup merge of rust-lang#122300 - CastilloDel:master, r=cjgillot

Add FileCheck annotations to mir-opt/dest-prop tests

Part of rust-lang#116971, adds FileCheck annotations to MIR-opt tests in tests/mir-opt/dest-prop.

I would like some feedback. Also, I don't know how to approach `union.rs`.  I couldn't figure out what it is testing.

r? cjgillot
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Jul 16, 2024
Add FileCheck annotations to mir-opt/dest-prop tests

Part of rust-lang/rust#116971, adds FileCheck annotations to MIR-opt tests in tests/mir-opt/dest-prop.

I would like some feedback. Also, I don't know how to approach `union.rs`.  I couldn't figure out what it is testing.

r? cjgillot
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 20, 2024
…s, r=Mark-Simulacrum

Check for filecheck directives in files marked `skip-filecheck`

cc rust-lang#116971
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 21, 2024
Rollup merge of rust-lang#131927 - clubby789:skip-filecheck-directives, r=Mark-Simulacrum

Check for filecheck directives in files marked `skip-filecheck`

cc rust-lang#116971
@jieyouxu jieyouxu added the E-tedious Call for participation: An issue involves lots of work and is better handled as many small tasks. label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt Area: MIR optimizations A-testsuite Area: The testsuite used to check the correctness of rustc E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. E-tedious Call for participation: An issue involves lots of work and is better handled as many small tasks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests