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

Make some const prop mir-opt tests unit-tests #99770

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

Noratrieb
Copy link
Member

Most of these have no or only tiny diffs beyond line numbers being changed (would it make sense to not have line numbers in mir-opt tests?). Some things changed a bit, but I think it should all be fine, not sure though.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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
@Noratrieb
Copy link
Member Author

r? @JakobDegen

@JakobDegen
Copy link
Contributor

would it make sense to not have line numbers in mir-opt tests?

This has been discussed a number of times, and people have mixed opinions on it. I still think we should do it though. I think there was a Zulip thread about this somewhere.

I'll review later as well, but no perms so Mark will need to review himself or hand off to mir-opt or something

@JakobDegen
Copy link
Contributor

I'm going to wait to review this until after #99780 lands, for obvious reasons

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 28, 2022
…-obk

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

As shown in rust-lang#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
@bors
Copy link
Contributor

bors commented Jul 28, 2022

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

Copy link
Contributor

@JakobDegen JakobDegen left a comment

Choose a reason for hiding this comment

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

Overall looks good. Unfortunately, const prop currently uses the mir-opt-level to determine how much constant propagation to do, which plays poorly with // unit-test since that emits -Z mir-opt-level=0. This causes a couple of regressions (see below) in places where const prop is now less aggressive because of the flag. Imo it's a bad idea to have the same flag control both whether other optimizations run and how aggressive const prop is. I think we should instead have -Z mir-const-prop-level which defaults to the same value as -Z mir-opt-level but can be set separately. Assuming Oli agrees with this assessment, after this is merged can you file a follow-up issue to that effect?

Edit: I hit this in an unrelated case, so I've gone and opened a Zulip thread about it

+ _3 = const 0_i32; // scope 1 at $DIR/bad_op_div_by_zero.rs:+2:18: +2:19
+ _4 = const true; // scope 1 at $DIR/bad_op_div_by_zero.rs:+2:14: +2:19
+ assert(!const true, "attempt to divide `{}` by zero", const 1_i32) -> bb1; // scope 1 at $DIR/bad_op_div_by_zero.rs:+2:14: +2:19
Copy link
Contributor

Choose a reason for hiding this comment

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

This is one example but probably non-critical

+ _4 = const 4_usize; // scope 2 at $DIR/boxes.rs:+1:14: +1:22
+ _5 = const 4_usize; // scope 2 at $DIR/boxes.rs:+1:14: +1:22
+ _6 = alloc::alloc::exchange_malloc(const 4_usize, const 4_usize) -> bb1; // scope 2 at $DIR/boxes.rs:+1:14: +1:22
Copy link
Contributor

Choose a reason for hiding this comment

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

Also non-critical here

@Noratrieb
Copy link
Member Author

I forgot about this PR :D, but fixed the one bad case now.

@Noratrieb
Copy link
Member Author

Noratrieb commented Aug 20, 2022

Let's reroll to get it through
r? rust-lang/mir-opt

@Noratrieb
Copy link
Member Author

r? rust-lang/mir-opt

@Noratrieb
Copy link
Member Author

it does not want to do the thing :/

@JakobDegen
Copy link
Contributor

Lets just

r? @wesleywiser

since he reviewed a similar PR a bit ago

@oli-obk
Copy link
Contributor

oli-obk commented Aug 22, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Aug 22, 2022

📌 Commit ee30cc8 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 Aug 22, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 22, 2022
…-obk

Make some const prop mir-opt tests `unit-test`s

Most of these have no or only tiny diffs beyond line numbers being changed (would it make sense to not have line numbers in mir-opt tests?). Some things changed a bit, but I think it should all be fine, not sure though.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2022
Rollup of 8 pull requests

Successful merges:

 - rust-lang#98200 (Expand potential inner `Or` pattern for THIR)
 - rust-lang#99770 (Make some const prop mir-opt tests `unit-test`s)
 - rust-lang#99957 (Rework Ipv6Addr::is_global to check for global reachability rather than global scope - rebase)
 - rust-lang#100331 (Guarantee `try_reserve` preserves the contents on error)
 - rust-lang#100336 (Fix two const_trait_impl issues)
 - rust-lang#100713 (Convert diagnostics in parser/expr to SessionDiagnostic)
 - rust-lang#100820 (Use pointer `is_aligned*` methods)
 - rust-lang#100872 (Add guarantee that Vec::default() does not alloc)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e77c208 into rust-lang:master Aug 22, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 22, 2022
@Noratrieb Noratrieb deleted the mir-pass-unit-test branch August 22, 2022 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants