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

Remove comments from mir-opt MIR dumps #112346

Merged
merged 1 commit into from Jun 16, 2023
Merged

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Jun 6, 2023

See https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/Line.20numbers.20in.20mir-opt.20tests/near/363849874

In #99780 there is mention that "there has been a zulip conversation about disabling line numbers with mixed opinions" which to me means that some people opposed this. I can't find the referenced conversation so... here we go.

The current situation is quite chaotic. It's not hard to find MIR diffs which contain

  • Absolute line numbers
  • Relative line numbers
  • Substituted line numbers (LL)
    For example:
    let mut _5: *mut std::option::Option<B>; // in scope 0 at $DIR/inline_shims.rs:+2:38: +2:39
    scope 1 {
    }
    scope 2 {
    + scope 3 (inlined std::ptr::drop_in_place::<Option<B>> - shim(Some(Option<B>))) { // at $DIR/inline_shims.rs:12:14: 12:40
    + let mut _6: isize; // in scope 3 at $SRC_DIR/core/src/ptr/mod.rs:LL:COL
    + let mut _7: isize; // in scope 3 at $SRC_DIR/core/src/ptr/mod.rs:LL:COL
    + }

And sometimes adding a comment at the top of a mir-opt test generates a diff in the test because a line number changed: https://github.com/rust-lang/rust/pull/98112/files#diff-b8cf4bcce95078e6a3faf075e9abf6864872fb28a64d95c04f04513b9e3bbd81

And irrelevant changes to the standard library can generate diffs in mir-opt tests: https://github.com/rust-lang/rust/pull/110694/files#diff-bf96b0e7c67b8b272814536888fd9428c314991e155beae1f0a2a67f0ac47b2c
769886c

I think we should, specifically in mir-opt tests, completely remove the comments, or insert placeholders for all line and column numbers.

@rustbot
Copy link
Collaborator

rustbot commented Jun 6, 2023

r? @eholk

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

@rustbot 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) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 6, 2023
Comment on lines 13 to 16
_2 = const {alloc2: *mut i32};
// mir::Constant
// + span: $DIR/consts.rs:28:38: 28:39
// + literal: Const { ty: *mut i32, val: Value(Scalar(alloc2)) }
Copy link
Member

Choose a reason for hiding this comment

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

Can you put the // mir::Constant on the same line as the _2 = const {alloc2: *mut i32};?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see I missed a few more places the comments are emitted.

@rust-log-analyzer

This comment has been minimized.

@saethlin saethlin force-pushed the no-comment branch 3 times, most recently from 5ff35a3 to 68d0af9 Compare June 6, 2023 18:06
@saethlin saethlin marked this pull request as ready for review June 6, 2023 21:28
@saethlin
Copy link
Member Author

saethlin commented Jun 6, 2023

r? mir-opt

@rustbot rustbot assigned oli-obk and unassigned eholk Jun 6, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 14, 2023

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

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 15, 2023

@bors r+ p=1 bitrotty

@bors
Copy link
Contributor

bors commented Jun 15, 2023

📌 Commit e2fab8f0dcca015d83c83a06f5f25eb368b7bf5e has been approved by oli-obk

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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 15, 2023
@bors
Copy link
Contributor

bors commented Jun 15, 2023

⌛ Testing commit e2fab8f0dcca015d83c83a06f5f25eb368b7bf5e with merge e72dff92be7b99d9222f4fdb685df5ca261c257a...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 15, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 15, 2023
@saethlin
Copy link
Member Author

I goofed. I forgot that even with all the awesome improvements to blessing that @pietroalbini built, blessing with debug = true is still not effective.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 15, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jun 15, 2023

📌 Commit 0a1fa41 has been approved by oli-obk

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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 15, 2023
@scottmcm
Copy link
Member

blessing with debug = true is still not effective.

Not this PR, but maybe emitting a warning for that could make sense?

(And I assume it's debug-std = true that's the major issue? debug = true; debug-std = false should be fine for blessing these?)

@saethlin
Copy link
Member Author

The problem is that the tests don't get blessed at all because they are // ignore-debug.

Yes, it's the standard library code that's the problem, and I think this can be gracefully solved by piggybacking on the work Pietro did.

@bors
Copy link
Contributor

bors commented Jun 16, 2023

⌛ Testing commit 0a1fa41 with merge c84d5e7...

@bors
Copy link
Contributor

bors commented Jun 16, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c84d5e7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 16, 2023
@bors bors merged commit c84d5e7 into rust-lang:master Jun 16, 2023
12 checks passed
@rustbot rustbot added this to the 1.72.0 milestone Jun 16, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c84d5e7): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)
-2.2% [-2.2%, -2.2%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.2% [-2.2%, -2.2%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.4% [1.1%, 4.5%] 6
Regressions ❌
(secondary)
1.4% [1.4%, 1.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.4% [1.1%, 4.5%] 6

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 649.004s -> 647.175s (-0.28%)

@saethlin saethlin deleted the no-comment branch June 27, 2023 02:23
@lqd
Copy link
Member

lqd commented Jul 18, 2023

It seems this PR removed comments from -Zdump-mir dumps that were not mir-opt tests. I'll open a PR.

@shepmaster
Copy link
Member

This also removed functionality from the playground. I'll delete that code now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet