Reduce MIR dump file count for MIR-opt tests #110773

mj10021 · 2023-04-24T16:03:53Z

As referenced in issue #109502 , mir-opt tests previously used the -Zdump-mir=all flag, which generates very large output. This PR only dumps the passes under test, greatly reducing dump output.

rustbot · 2023-04-24T16:04:00Z

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

oli-obk

just a bunch of impl nits.

Please also add some comments explaining the new field and argument

src/tools/compiletest/src/runtest.rs

src/tools/miropt-test-tools/src/lib.rs

oli-obk · 2023-04-27T21:41:42Z

src/tools/miropt-test-tools/src/lib.rs

                expected_file =
                    format!("{}{}{}", test_name.trim_end_matches(extension), bit_width, extension,);
                from_file = test_name.to_string();
                assert!(test_names.next().is_none(), "two mir pass names specified for MIR dump");
                to_file = None;
+                let temp = test_name.split('.').collect::<Vec<&str>>();
+                passes.push(temp[temp.len() - 3].to_string());


What does this magic number 3 mean? (Add the answer as a comment to the code)

Haha got it, adding

You could also use rsplit and the nth(3) operation on the iterator

Is the comment I added good or is there more info that is missing?

Ugh sorry, github didn't send my comment because I used a review

O no worries, thanks for the update 🙂

oli-obk · 2023-05-03T11:17:07Z

src/tools/miropt-test-tools/src/lib.rs

                let test_against = format!("{}.after.mir", trimmed);
                from_file = format!("{}.before.mir", trimmed);
                expected_file = format!("{}{}.diff", trimmed, bit_width);
                assert!(test_names.next().is_none(), "two mir pass names specified for MIR diff");
                to_file = Some(test_against);
            } else if let Some(first_pass) = test_names.next() {
                let second_pass = test_names.next().unwrap();
+                passes.push(first_pass.split('.').collect::<Vec<&str>>()[0].to_string());


You should be able to use split_once here to avoid collecting or iterating

oli-obk · 2023-05-03T11:18:42Z

src/tools/miropt-test-tools/src/lib.rs

                expected_file =
                    format!("{}{}{}", test_name.trim_end_matches(extension), bit_width, extension,);
                from_file = test_name.to_string();
                assert!(test_names.next().is_none(), "two mir pass names specified for MIR dump");
                to_file = None;
+                let temp = test_name.split('.').collect::<Vec<&str>>();
+                passes.push(temp[temp.len() - 3].to_string());


You could also use rsplit and the nth(3) operation on the iterator

oli-obk · 2023-05-04T10:11:07Z

src/tools/miropt-test-tools/src/lib.rs

                expected_file =
                    format!("{}{}{}", test_name.trim_end_matches(extension), bit_width, extension,);
                from_file = test_name.to_string();
                assert!(test_names.next().is_none(), "two mir pass names specified for MIR dump");
                to_file = None;
+                let temp = test_name.split('.').collect::<Vec<&str>>();
+                passes.push(temp[temp.len() - 3].to_string());


Ugh sorry, github didn't send my comment because I used a review

rustbot · 2023-05-04T22:51:50Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

mj10021 · 2023-05-04T22:55:13Z

oops fixing

blessed new test

oli-obk · 2023-05-05T08:46:13Z

Thanks!

@bors r+ rollup

bors · 2023-05-05T08:46:15Z

📌 Commit c19959f has been approved by oli-obk

It is now in the queue for this repository.

klensy · 2023-05-05T13:28:15Z

src/tools/compiletest/src/runtest.rs

-        let proc_res = self.compile_test(should_run, self.should_emit_metadata(pm));
+        let proc_res = self.compile_test(should_run, self.should_emit_metadata(pm), Vec::new());


Why changing compile_test function at all, if in all places except one, last parameter will be Vec::new()? You can just add something like compile_test_with_passes and use it exactly in that one place.

good point, I'll make a follow up with that change

klensy · 2023-05-05T13:31:54Z

tests/mir-opt/const_allocation.main.ConstProp.after.32bit.mir

-    ╾─alloc18─╼ 03 00 00 00                         │ ╾──╼....
+    ╾─alloc19─╼ 03 00 00 00                         │ ╾──╼....


Why this affects alloc numbers at all?

Rollup of 6 pull requests Successful merges: - rust-lang#103056 (Fix `checked_{add,sub}_duration` incorrectly returning `None` when `other` has more than `i64::MAX` seconds) - rust-lang#108801 (Implement RFC 3348, `c"foo"` literals) - rust-lang#110773 (Reduce MIR dump file count for MIR-opt tests) - rust-lang#110876 (Added default target cpu to `--print target-cpus` output and updated docs) - rust-lang#111068 (Improve check-cfg implementation) - rust-lang#111238 (btree_map: `Cursor{,Mut}::peek_prev` must agree) Failed merges: - rust-lang#110694 (Implement builtin # syntax and use it for offset_of!(...)) r? `@ghost` `@rustbot` modify labels: rollup

…oli-obk Issue 109502 follow up, remove unnecessary Vec::new() from compile_test() As mentioned in comment on PR rust-lang#110773 , adding a separate function to pass the test passes into the `dump-mir` is a bit nicer

rustbot assigned oli-obk Apr 24, 2023

rustbot added 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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 24, 2023

This comment has been minimized.

Sign in to view

mj10021 force-pushed the issue-109502-fix branch from 4a21da9 to 0766d14 Compare April 24, 2023 19:07

This comment has been minimized.

Sign in to view

mj10021 force-pushed the issue-109502-fix branch from 0766d14 to 47144a2 Compare April 25, 2023 01:31

oli-obk requested changes Apr 25, 2023

View reviewed changes

src/tools/compiletest/src/runtest.rs Outdated Show resolved Hide resolved

src/tools/compiletest/src/runtest.rs Outdated Show resolved Hide resolved

src/tools/compiletest/src/runtest.rs Outdated Show resolved Hide resolved

mj10021 force-pushed the issue-109502-fix branch from 47144a2 to b4714ae Compare April 25, 2023 22:57

oli-obk reviewed Apr 27, 2023

View reviewed changes

src/tools/miropt-test-tools/src/lib.rs Outdated Show resolved Hide resolved

oli-obk reviewed Apr 27, 2023

View reviewed changes

mj10021 force-pushed the issue-109502-fix branch 2 times, most recently from fe36aec to 0812a49 Compare April 27, 2023 23:28

oli-obk requested changes May 4, 2023

View reviewed changes

mj10021 force-pushed the issue-109502-fix branch from 0812a49 to 1056ff3 Compare May 4, 2023 22:51

add passes to miroptfiles struct and passed to -zdump-mir args

c19959f

blessed new test

mj10021 force-pushed the issue-109502-fix branch from 1056ff3 to c19959f Compare May 4, 2023 22:56

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-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2023

Dylan-DPC mentioned this pull request May 5, 2023

Rollup of 6 pull requests #111248

Merged

klensy reviewed May 5, 2023

View reviewed changes

mj10021 mentioned this pull request May 5, 2023

Issue 109502 follow up, remove unnecessary Vec::new() from compile_test() #111251

Merged

bors merged commit de7e29e into rust-lang:master May 5, 2023

rustbot added this to the 1.71.0 milestone May 5, 2023

mj10021 deleted the issue-109502-fix branch November 1, 2024 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce MIR dump file count for MIR-opt tests #110773

Reduce MIR dump file count for MIR-opt tests #110773

mj10021 commented Apr 24, 2023

rustbot commented Apr 24, 2023

This comment has been minimized.

This comment has been minimized.

oli-obk left a comment

oli-obk Apr 27, 2023

mj10021 Apr 27, 2023

oli-obk May 3, 2023

mj10021 May 3, 2023

oli-obk May 4, 2023

mj10021 May 4, 2023

oli-obk May 3, 2023

oli-obk May 3, 2023

oli-obk May 4, 2023

rustbot commented May 4, 2023

mj10021 commented May 4, 2023

oli-obk commented May 5, 2023

bors commented May 5, 2023

klensy May 5, 2023

mj10021 May 5, 2023

klensy May 5, 2023

		let proc_res = self.compile_test(should_run, self.should_emit_metadata(pm));
		let proc_res = self.compile_test(should_run, self.should_emit_metadata(pm), Vec::new());

		╾─alloc18─╼ 03 00 00 00 │ ╾──╼....
		╾─alloc19─╼ 03 00 00 00 │ ╾──╼....

Reduce MIR dump file count for MIR-opt tests #110773

Reduce MIR dump file count for MIR-opt tests #110773

Conversation

mj10021 commented Apr 24, 2023

rustbot commented Apr 24, 2023

This comment has been minimized.

This comment has been minimized.

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented May 4, 2023

mj10021 commented May 4, 2023

oli-obk commented May 5, 2023

bors commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment