Skip to content

Add FileCheck annotations to mir-opt/copy-prop #135099

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Add FileCheck annotations to mir-opt/copy-prop #135099

wants to merge 20 commits into from
+119 −16

Conversation

Shunpoco
Copy link
Contributor

@Shunpoco Shunpoco commented Jan 4, 2025

This resolves a part of #116971 .

This PR adds FileCheck annotations to test files under mir-opt/copy-prop.

Shunpoco added 14 commits January 4, 2025 11:18
@Shunpoco
Add FileCheck to branch.rs
1b5a744 
Signed-off-by: Shunpoco <[email protected]>
@Shunpoco
Add FileCheck to calls.rs
e6dcb25 
Signed-off-by: Shunpoco <[email protected]>
@Shunpoco
Add FileCheck to copy_propagation_arg.rs
78a143f 
Signed-off-by: Shunpoco <[email protected]>
@Shunpoco
Add FileCheck to custom_move_arg.rs
ab21ba3 
Signed-off-by: Shunpoco <[email protected]>
@Shunpoco
Add FileCheck to cycle.rs
4057797 
Signed-off-by: Shunpoco <[email protected]>
@Shunpoco
Add FileCheck to dead_stores_79191.rs
77d5711 
Signed-off-by: Shunpoco <[email protected]>
@Shunpoco
Add FileCheck to dead_stores_better.rs
f53476b 
Signed-off-by: Shunpoco <[email protected]>
@Shunpoco
Add FileCheck to issue_107511.rs
2d04a63 
Signed-off-by: Shunpoco <[email protected]>
@Shunpoco
Add FileCheck to move_arg.rs
d0365d3 
Signed-off-by: Shunpoco <[email protected]>
@Shunpoco
Add FileCheck to move_projection.rs
334d501 
Signed-off-by: Shunpoco <[email protected]>
@Shunpoco
Add FileCheck to mutate_through_pointer.rs
d3176d5 
Signed-off-by: Shunpoco <[email protected]>
@Shunpoco
Add FileCheck to non_dominate.rs
ef42830 
Signed-off-by: Shunpoco <[email protected]>
@Shunpoco
Add FileCheck to partial_init.rs
4627ea1 
Signed-off-by: Shunpoco <[email protected]>
@Shunpoco
Add FileCheck to reborrow.rs
381466c 
Signed-off-by: Shunpoco <[email protected]>
@rustbot
Copy link
Collaborator

rustbot commented Jan 4, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 4, 2025
Shunpoco
Shunpoco commented Jan 4, 2025
View reviewed changes
tests/mir-opt/copy-prop/copy_propagation_arg.rs
dummy(x);
x = 5;
}

// EMIT_MIR copy_propagation_arg.baz.CopyProp.diff
fn baz(mut x: i32) -> i32 {
// CHECK-LABEL: fn baz(
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'm wondering if we don't need this function for CopyProp.
I guess this test file originally from dest-prop, and in dest-prop the self-assignment is eliminated. But in copy-prop, it is not eliminated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. Do you mind fixing the comment and adding a CHECK: [[x]] = copy [[x]];?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and adding a CHECK: [[x]] = copy [[x]];?

There is no self-copy instruction in the mir file, but it has _2 = copy _1 and _0 = copy _1 (I already added).
Do you mean that I should add CHECK: _2 = copy [[x]];?

Copy link
Contributor Author

@Shunpoco Shunpoco Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I modified it in 7a64137. Please check if the change matches with your thought 🙏

@Shunpoco Shunpoco marked this pull request as ready for review January 4, 2025 13:16
@Shunpoco
Copy link
Contributor Author

Shunpoco commented Jan 4, 2025

r? @cjgillot

@rustbot rustbot assigned cjgillot and unassigned Mark-Simulacrum Jan 4, 2025
cjgillot
cjgillot reviewed Jan 8, 2025
View reviewed changes
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.

Thanks a lot for the great work. A few nits.

tests/mir-opt/copy-prop/copy_propagation_arg.rs
dummy(x);
x = 5;
}

// EMIT_MIR copy_propagation_arg.baz.CopyProp.diff
fn baz(mut x: i32) -> i32 {
// CHECK-LABEL: fn baz(
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. Do you mind fixing the comment and adding a CHECK: [[x]] = copy [[x]];?

@apiraino
Copy link
Contributor

apiraino commented Apr 8, 2025

r? compiler

@rustbot rustbot assigned BoxyUwU and unassigned cjgillot Apr 8, 2025
@BoxyUwU
Copy link
Member

BoxyUwU commented Apr 8, 2025

r? @jieyouxu
weird test stuff

@rustbot rustbot assigned jieyouxu and unassigned BoxyUwU Apr 8, 2025
jieyouxu
jieyouxu reviewed Apr 8, 2025
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

The FileCheck annotations syntactically look okay but I don't know mir-opts well so rerolling a mir-opt reviewer.

@jieyouxu
Copy link
Member

jieyouxu commented Apr 8, 2025

r? mir-opt

@rustbot rustbot assigned wesleywiser and unassigned jieyouxu Apr 8, 2025
@jieyouxu jieyouxu added A-testsuite Area: The testsuite used to check the correctness of rustc A-mir-opt Area: MIR optimizations labels Apr 8, 2025
wesleywiser
wesleywiser reviewed Apr 18, 2025
View reviewed changes
tests/mir-opt/copy-prop/dead_stores_better.rs
// EMIT_MIR_FOR_EACH_PANIC_STRATEGY
// This is a copy of the `dead_stores_79191` test, except that we turn on DSE. This demonstrates
// that that pass enables this one to do more optimizations.

//@ test-mir-pass: CopyProp
//@ compile-flags: -Zmir-enable-passes=+DeadStoreElimination
//@ compile-flags: -Zmir-enable-passes=+DeadStoreElimination-initial
Copy link
Member

Choose a reason for hiding this comment

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

Currious why the change here? The CHECK statements look identical to those in dead_stores_79191.rs which makes the comment above outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Now I confirmed that mir files from those tests are identical.
They were introduced in #106908, but in the PR the .f.CopyProp.after.mir files already looked identical.
Should we remove this test as duplicated test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wesleywiser wesleywiser wesleywiser left review comments

@cjgillot cjgillot cjgillot left review comments

@jieyouxu jieyouxu jieyouxu left review comments

Assignees

@wesleywiser wesleywiser

Labels
A-mir-opt Area: MIR optimizations A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Shunpoco @rustbot @apiraino @BoxyUwU @jieyouxu @wesleywiser @cjgillot @Mark-Simulacrum