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.

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

Use line numbers relative to the function in mir-opt tests #99780

Merged
merged 2 commits into from
Jul 28, 2022
Merged

Use line numbers relative to the function in mir-opt tests #99780

merged 2 commits into from
Jul 28, 2022

Conversation

Nilstrieb
Copy link
Member

@Nilstrieb Nilstrieb commented Jul 26, 2022 •
edited
Loading

As shown in #99770, the line numbers can be a big source of needless and confusing diffs. This PR adds a new flag -Zmir-pretty-relative-line-numbers to make them relative to the function declaration, which avoids most needless diffs from attribute changes.

@JakobDegen told me that there has been a zulip conversation about disabling line numbers with mixed opinions, so I'd like to get some feedback here, for this hopefully better solution.

r? rust-lang/wg-mir-opt

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 26, 2022
@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 26, 2022
@Nilstrieb
Copy link
Member Author

r? rust-lang/mir-opt

@rust-highfive rust-highfive assigned oli-obk and unassigned estebank Jul 26, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@klensy
Copy link
Contributor

klensy commented Jul 26, 2022

Maybe reuse already existing style like LL:COL instead of $LINE:$COL?:

rust/src/test/mir-opt/const_prop/control_flow_simplification.hello.ConstProp.diff

Line 18 in c11207e

StorageLive(_2); // scope 0 at $SRC_DIR/std/src/panic.rs:LL:COL

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2022

Maybe we could make this info relative to the function start? That should avoid most of the noise, right?

@Nilstrieb
Copy link
Member Author

Nilstrieb commented Jul 27, 2022 •
edited
Loading

This should avoid most noise, yes. It would probably be a bit tricky to implement with the current code structure, but I could do it. What do you think would be the best way to solve this? Rename -Zmir-pretty-line-numbers to something like -Zmir-pretty-relative-line-numbers and adding a new method on source_map for relative line numbers? The span of the function should be available in mir pretty printing I think.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2022

Rename -Zmir-pretty-line-numbers to something like-Zmir-pretty-relative-line-numbersand adding a new method onsource_map` for relative line numbers?

that sounds reasonable to me

@Nilstrieb Nilstrieb changed the title Don't show line numbers in mir-opt test diffs Use line numbers relative to the function in mir-opt tests Jul 27, 2022
@bors
Copy link
Contributor

bors commented Jul 27, 2022

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

@JakobDegen JakobDegen mentioned this pull request Jul 28, 2022
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
oli-obk
oli-obk requested changes Jul 28, 2022
View reviewed changes
compiler/rustc_span/src/source_map.rs Show resolved Hide resolved
compiler/rustc_span/src/source_map.rs Outdated Show resolved Hide resolved
@Nilstrieb Nilstrieb force-pushed the mir-opt-test-line-no branch 2 times, most recently from 3709c2a to ba0af95 Compare July 28, 2022 09:02
@oli-obk
Copy link
Contributor

oli-obk commented Jul 28, 2022

@bors r+ p=1 bitrotty

@bors
Copy link
Contributor

bors commented Jul 28, 2022

📌 Commit ba0af9561870ee2ba942f48a31ed0260f3cb3469 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 Jul 28, 2022
@bors
Copy link
Contributor

bors commented Jul 28, 2022

⌛ Testing commit ba0af9561870ee2ba942f48a31ed0260f3cb3469 with merge 2cba4390459755873c122e7fe462bcc216572595...

@Nilstrieb
Copy link
Member Author

I figured it out. Maybe it works now.

@Dylan-DPC
Copy link
Member

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jul 28, 2022

📌 Commit 11c0280 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 Jul 28, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Dylan-DPC
Copy link
Member

@bors r-

@Nilstrieb
Copy link
Member Author

I think you're a bit late my friend, but thank you for your log analysis anyways :)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 28, 2022
@Dylan-DPC
Copy link
Member

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jul 28, 2022

📌 Commit 11c0280 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 28, 2022
@Nilstrieb Nilstrieb mentioned this pull request Jul 28, 2022
Open
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jul 28, 2022

⌛ Testing commit 11c0280 with merge 36f4f4a...

@bors
Copy link
Contributor

bors commented Jul 28, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 28, 2022
@bors bors merged commit 36f4f4a into rust-lang:master Jul 28, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 28, 2022
@Nilstrieb Nilstrieb deleted the mir-opt-test-line-no branch July 28, 2022 19:12
This was referenced Jul 28, 2022
Merged
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (36f4f4a): comparison url.

Instruction count

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

Max RSS (memory usage)

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 3.0% 3.0% 1
Regressions 😿
(secondary)		 2.0% 2.0% 1
Improvements 🎉
(primary)		 -2.1% -3.0% 2
Improvements 🎉
(secondary)		 -3.1% -3.1% 1
All 😿🎉 (primary) -0.4% 3.0% 3

Cycles

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)		 2.5% 2.5% 1
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 2.5% 2.5% 1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2

  2. number of relevant changes ↩ ↩2

@saethlin saethlin mentioned this pull request Jun 6, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 16, 2023
@bors
Auto merge of rust-lang#112346 - saethlin:no-comment, r=oli-obk
c84d5e7 
Remove comments from mir-opt MIR dumps

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

In rust-lang#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: https://github.com/rust-lang/rust/blob/408bbd040613f6776e0a7d05d582c81f4ddc189a/tests/mir-opt/inline/inline_shims.drop.Inline.diff#L10-L17

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
rust-lang@769886c

I think we should, specifically in mir-opt tests, completely remove the comments, or insert placeholders for all line and column numbers.
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Jun 16, 2023
@bors
Auto merge of #112346 - saethlin:no-comment, r=oli-obk
44ed9eb 
Remove comments from mir-opt MIR dumps

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

In rust-lang/rust#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: https://github.com/rust-lang/rust/blob/408bbd040613f6776e0a7d05d582c81f4ddc189a/tests/mir-opt/inline/inline_shims.drop.Inline.diff#L10-L17

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
rust-lang/rust@769886c

I think we should, specifically in mir-opt tests, completely remove the comments, or insert placeholders for all line and column numbers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk requested changes

Assignees

@oli-obk oli-obk

Labels
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.64.0
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@Nilstrieb @rust-highfive @rust-log-analyzer @klensy @oli-obk @bors @Dylan-DPC @rust-timer @estebank @rustbot