-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #123322) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this 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?
tests/mir-opt/dest-prop/branch.rs
Outdated
@@ -12,6 +11,8 @@ fn cond() -> bool { | |||
|
|||
// EMIT_MIR branch.foo.DestinationPropagation.diff | |||
fn foo() -> i32 { | |||
// CHECK-LABEL: fn foo | |||
// CHECK-NOT: {{_.*}} = {{_.*}} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
I went over all of them, trying to improve them with what you said.
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 ( |
This comment has been minimized.
This comment has been minimized.
6774e8a
to
7ecf674
Compare
This comment has been minimized.
This comment has been minimized.
@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]] = {{_.*}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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]]; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 {{_.*}}) |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
hi. Please mark this PR as ready when you are finished, otherwise it won't appear in my todo list. |
@bors r+ rollup |
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
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
…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
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
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
…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
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
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