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/dest-prop tests #122300

Merged
merged 6 commits into from
Jul 14, 2024

Conversation

CastilloDel
Copy link
Contributor

@CastilloDel CastilloDel commented Mar 10, 2024

Part of #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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 3, 2024

☔ The latest upstream changes (presumably #123322) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

In a general manner, the checks you add {{_.*}} = {{_.*}} are very underspecified. Why copying and allow moving? Why only bare locals and not projections?

Could you make them more specific, for instance with the help of debug statements to identify locals by name.

I don't understand your remark about union.rs. There is a (small) summary of what should be tested. If you can't understand what it does, why did you change what it does? For the background, did you find which PR introduced it?

@@ -12,6 +11,8 @@ fn cond() -> bool {

// EMIT_MIR branch.foo.DestinationPropagation.diff
fn foo() -> i32 {
// CHECK-LABEL: fn foo
// CHECK-NOT: {{_.*}} = {{_.*}}
Copy link
Contributor

Choose a reason for hiding this comment

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

The check needs to be more specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope it's better now

@@ -9,25 +8,35 @@ fn dummy(x: u8) -> u8 {

// EMIT_MIR copy_propagation_arg.foo.DestinationPropagation.diff
fn foo(mut x: u8) {
// CHECK: foo
Copy link
Contributor

Choose a reason for hiding this comment

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

Use CHECK-LABEL for function names.

@cjgillot cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 6, 2024
@CastilloDel
Copy link
Contributor Author

Could you make them more specific, for instance with the help of debug statements to identify locals by name.

I went over all of them, trying to improve them with what you said.

I don't understand your remark about union.rs. There is a (small) summary of what should be tested. If you can't understand what it does, why did you change what it does? For the background, did you find which PR introduced it?

I found this PR, but I still couldn't understand. After reading the summary "Tests that we can propagate into places that are projections into unions" I would understand the test would check a propagation is performed correctly, but in the MIR (union.main.DestinationPropagation.panic-abort.diff) I don't see any changes, so I'm not sure how the test works. That's why I tried changing it to a unit test, but I'm still lost as to how to approach it.

@rust-log-analyzer

This comment has been minimized.

@CastilloDel CastilloDel force-pushed the master branch 2 times, most recently from 6774e8a to 7ecf674 Compare April 14, 2024 10:04
@rust-log-analyzer

This comment has been minimized.

@CastilloDel
Copy link
Contributor Author

@cjgillot Could you take another look when you have time? Thanks

dummy(x);
x = 5;
}

// EMIT_MIR copy_propagation_arg.baz.DestinationPropagation.diff
fn baz(mut x: i32) -> i32 {
// CHECK-LABEL: fn baz(
// CHECK: debug x => [[x:_.*]];
// CHECK-NOT: [[x]] = {{_.*}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// CHECK-NOT: [[x]] = {{_.*}}
// CHECK-NOT: [[x]] =

Just to be on the safe side.

// CHECK: debug y => [[y:_.*]];
// CHECK: [[y]] = [[x]]
// CHECK: [[x]] = const 123_i32;
// CHECK-NOT: {{_.*}} = [[y]];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we directly give them their names, aka x is _1 and y is _0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm for me, it feels easier to read the test because you can more easily read the checks directly referencing the variable names (x and y), but maybe I'm misunderstanding something.

// CHECK: debug a => [[a:_.*]];
// CHECK: debug b => [[b:_.*]];
// CHECK: [[b]] = [[a]];
// CHECK: [[a]] = const 5_usize;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't DSE have removed this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was surprised too, but the test was like this

// CHECK-LABEL: fn nrvo(
// CHECK: debug init => [[init:_.*]];
// CHECK-NOT: {{_.*}} = [[init]];
// CHECK: move [[init]](move {{_.*}})
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, the point of this test is to track what happens to buf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed that, because there were no changes to buf. I added some more checks, let me know if it's better.

@cjgillot
Copy link
Contributor

@cjgillot Could you take another look when you have time? Thanks

hi. Please mark this PR as ready when you are finished, otherwise it won't appear in my todo list.

@CastilloDel CastilloDel marked this pull request as ready for review June 26, 2024 17:40
@cjgillot
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 13, 2024

📌 Commit 2d5a483 has been approved by cjgillot

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 13, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request 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 pull request 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
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 13, 2024
…kingjubilee

Rollup of 8 pull requests

Successful merges:

 - rust-lang#122300 (Add FileCheck annotations to mir-opt/dest-prop tests)
 - rust-lang#126967 (Promote the `wasm32-wasip2` target to Tier 2)
 - rust-lang#127370 (Windows: Add experimental support for linking std-required system DLLs using raw-dylib)
 - rust-lang#127434 (use "bootstrap" instead of "rustbuild" in comments and docs)
 - rust-lang#127477 (Clear `inner_attr_ranges` regularly.)
 - rust-lang#127558 (More attribute cleanups)
 - rust-lang#127659 (Use ManuallyDrop in BufWriter::into_parts)
 - rust-lang#127677 (using correct tool mode for `run-make-support` crate)

r? `@ghost`
`@rustbot` modify labels: rollup
jhpratt added a commit to jhpratt/rust that referenced this pull request 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
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#122300 (Add FileCheck annotations to mir-opt/dest-prop tests)
 - rust-lang#127153 (Initial implementation of anonymous_pipe API)
 - rust-lang#127434 (use "bootstrap" instead of "rustbuild" in comments and docs)
 - rust-lang#127477 (Clear `inner_attr_ranges` regularly.)
 - rust-lang#127659 (Use ManuallyDrop in BufWriter::into_parts)
 - rust-lang#127671 (rustdoc: rename `issue-\d+.rs` tests to have meaningful names (part 8))
 - rust-lang#127677 (using correct tool mode for `run-make-support` crate)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2024
…kingjubilee

Rollup of 6 pull requests

Successful merges:

 - rust-lang#122300 (Add FileCheck annotations to mir-opt/dest-prop tests)
 - rust-lang#127434 (use "bootstrap" instead of "rustbuild" in comments and docs)
 - rust-lang#127477 (Clear `inner_attr_ranges` regularly.)
 - rust-lang#127558 (More attribute cleanups)
 - rust-lang#127659 (Use ManuallyDrop in BufWriter::into_parts)
 - rust-lang#127671 (rustdoc: rename `issue-\d+.rs` tests to have meaningful names (part 8))

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4c493db into rust-lang:master Jul 14, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants