Normalize MIR with RevealAll before optimizations. #85254

cjgillot · 2021-05-13T11:42:05Z

rust-highfive · 2021-05-13T11:42:08Z

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

bjorn3 · 2021-05-13T12:05:43Z

Would the performance effect of always doing this be positive or negative? Normalizing during mir opt and during codegen may increase the amount of work, but it may also make the second normalization cheaper which could be a net win for generic functions.

lcnr · 2021-05-24T18:14:46Z

I am not able to review any PRs in the near future.

r? @ecstatic-morse

tmiasko · 2021-06-11T20:16:12Z

FYI: This introduces new test failures related to validation in sanitize_witness when building with: ./x.py test --rustc-args -Zinline-mir src/test/ui/async-await/

bors · 2021-06-22T01:24:32Z

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

lcnr · 2021-06-23T12:26:42Z

going to reassign this to myself, considering that I am slowly starting to become active again

r? @lcnr

lcnr

I think this is fine to land and might even be a perf improvement if enabled by default.

@cjgillot did you have the time to look into the test failure mentioned in #85254 (comment)?

lcnr · 2021-06-23T12:29:40Z

compiler/rustc_mir/src/transform/reveal_all.rs

+impl<'tcx> MirPass<'tcx> for RevealAll {
+    fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
+        // This pass must run before inlining, since we insert callee bodies in RevealAll mode.
+        if tcx.sess.mir_opt_level() < 3 && !super::inline::is_enabled(tcx) {


Suggested change

if tcx.sess.mir_opt_level() < 3 && !super::inline::is_enabled(tcx) {

if tcx.sess.mir_opt_level() >= 3 || super::inline::is_enabled(tcx) {

let param_env = tcx.param_env_reveal_all_normalized(body.source.def_id());

RevealAllVisitor { tcx, param_env }.visit_body(body);

}

seems slightly more understandable for me

lcnr · 2021-06-23T12:32:50Z

compiler/rustc_mir/src/transform/reveal_all.rs

+    }
+
+    #[inline]
+    fn process_projection_elem(


instead of overwriting process_projection_elem, would it make sense to instead update the default impl to always update the type? Seems a bit scary to not visit that one by default.

bjorn3 · 2021-07-10T16:16:56Z

Won't this break specialization by normalizing associated types when a specialization may cause a different type to be used?

lcnr · 2021-07-12T19:14:00Z

Won't this break specialization by normalizing associated types when a specialization may cause a different type to be used?

i would assume no 🤔 we shouldn't be able to normailze associated types if they are still specializable. So they remain unchanged in that case.

cjgillot · 2021-07-14T16:35:12Z

FYI: This introduces new test failures related to validation in sanitize_witness when building with: ./x.py test --rustc-args -Zinline-mir src/test/ui/async-await/

Latest CI runs show this ICE. I have no idea what is happening.

tmiasko · 2021-07-14T19:51:44Z

FYI: This introduces new test failures related to validation in sanitize_witness when building with: ./x.py test --rustc-args -Zinline-mir src/test/ui/async-await/

Latest CI runs show this ICE. I have no idea what is happening.

What happens now, I think, is that a generator witness is not normalized, while locals declaration are.

When I originally mentioned the issue, I was thinking about resolving this by disabling inlining into generators altogether, rather than tinkering with the validation in sanitize_witness.

crlf0710 · 2021-07-31T18:47:49Z

@cjgillot Ping from triage, CI is still red here. Mind fixing that?

JohnCSimon · 2021-09-06T05:04:14Z

@cjgillot Ping again from triage, CI is still red here

cjgillot · 2021-10-23T17:09:02Z

@bors r=lcnr

bors · 2021-10-23T17:09:03Z

📌 Commit 70aeced has been approved by lcnr

Normalize MIR with RevealAll before optimizations. Fixes rust-lang#78442

bors · 2021-10-24T11:04:57Z

⌛ Testing commit 70aeced with merge ed08a67...

bors · 2021-10-24T14:12:04Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing ed08a67 to master...

rust-timer · 2021-10-24T15:33:22Z

Finished benchmarking commit (ed08a67): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

Run reveal_all on MIR when inlining is activated. Fix logic error in rust-lang#85254 which prevented the pass from running when needed. Fixes rust-lang#78442 r? `@lcnr`

Run reveal_all on MIR when inlining is activated. Fix logic error in rust-lang#85254 which prevented the pass from running when needed. Fixes rust-lang#78442 r? ``@lcnr``

rust-highfive assigned lcnr May 13, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 13, 2021

rust-highfive assigned ecstatic-morse and unassigned lcnr May 24, 2021

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2021

rust-highfive assigned lcnr and unassigned ecstatic-morse Jun 23, 2021

lcnr reviewed Jun 23, 2021

View reviewed changes

crlf0710 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2021

cjgillot force-pushed the reveal-mir branch from 75d7e6d to 8deb308 Compare July 10, 2021 16:13

This comment has been minimized.

Sign in to view

crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 31, 2021

JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 15, 2021

This comment has been minimized.

Sign in to view

cjgillot force-pushed the reveal-mir branch from 3c346dc to a90ceab Compare October 17, 2021 13:47

This comment has been minimized.

Sign in to view

cjgillot force-pushed the reveal-mir branch from a90ceab to 70aeced Compare October 17, 2021 14:33

Ignore wasm32 in test.

70aeced

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 Oct 23, 2021

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 23, 2021

Rollup merge of rust-lang#85254 - cjgillot:reveal-mir, r=lcnr

41b215b

Normalize MIR with RevealAll before optimizations. Fixes rust-lang#78442

matthiaskrgr mentioned this pull request Oct 23, 2021

Rollup of 3 pull requests #90217

Closed

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 24, 2021

Rollup merge of rust-lang#85254 - cjgillot:reveal-mir, r=lcnr

3155b7c

Normalize MIR with RevealAll before optimizations. Fixes rust-lang#78442

matthiaskrgr mentioned this pull request Oct 24, 2021

Rollup of 5 pull requests #90222

Closed

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 24, 2021

Rollup merge of rust-lang#85254 - cjgillot:reveal-mir, r=lcnr

5e2d876

Normalize MIR with RevealAll before optimizations. Fixes rust-lang#78442

This was referenced Oct 24, 2021

Rollup of 5 pull requests #90224

Closed

Rollup of 6 pull requests #90226

Closed

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 24, 2021

Rollup merge of rust-lang#85254 - cjgillot:reveal-mir, r=lcnr

433ecd1

Normalize MIR with RevealAll before optimizations. Fixes rust-lang#78442

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 24, 2021

bors merged commit ed08a67 into rust-lang:master Oct 24, 2021

rustbot added this to the 1.58.0 milestone Oct 24, 2021

cjgillot deleted the reveal-mir branch October 24, 2021 18:31

Alexendoo mentioned this pull request Nov 1, 2021

ICE: broken mir with -Zinline-mir issue-50865-private-impl-trait/auxiliary/lib.rs #78442

Closed

cjgillot mentioned this pull request Nov 6, 2021

Run reveal_all on MIR when inlining is activated. #90649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize MIR with RevealAll before optimizations. #85254

Normalize MIR with RevealAll before optimizations. #85254

cjgillot commented May 13, 2021

rust-highfive commented May 13, 2021

bjorn3 commented May 13, 2021

lcnr commented May 24, 2021

tmiasko commented Jun 11, 2021

bors commented Jun 22, 2021

lcnr commented Jun 23, 2021

lcnr left a comment

lcnr Jun 23, 2021

lcnr Jun 23, 2021

bjorn3 commented Jul 10, 2021

This comment has been minimized.

lcnr commented Jul 12, 2021

cjgillot commented Jul 14, 2021

tmiasko commented Jul 14, 2021 •

edited

Loading

crlf0710 commented Jul 31, 2021

JohnCSimon commented Sep 6, 2021

This comment has been minimized.

This comment has been minimized.

cjgillot commented Oct 23, 2021

bors commented Oct 23, 2021

bors commented Oct 24, 2021

bors commented Oct 24, 2021

rust-timer commented Oct 24, 2021

-        if tcx.sess.mir_opt_level() < 3 && !super::inline::is_enabled(tcx) {
+        if tcx.sess.mir_opt_level() >= 3 || super::inline::is_enabled(tcx) {
+            let param_env = tcx.param_env_reveal_all_normalized(body.source.def_id());
+            RevealAllVisitor { tcx, param_env }.visit_body(body);
+        }

Normalize MIR with RevealAll before optimizations. #85254

Normalize MIR with RevealAll before optimizations. #85254

Conversation

cjgillot commented May 13, 2021

rust-highfive commented May 13, 2021

bjorn3 commented May 13, 2021

lcnr commented May 24, 2021

tmiasko commented Jun 11, 2021

bors commented Jun 22, 2021

lcnr commented Jun 23, 2021

lcnr left a comment

Choose a reason for hiding this comment

lcnr Jun 23, 2021

Choose a reason for hiding this comment

lcnr Jun 23, 2021

Choose a reason for hiding this comment

bjorn3 commented Jul 10, 2021

This comment has been minimized.

lcnr commented Jul 12, 2021

cjgillot commented Jul 14, 2021

tmiasko commented Jul 14, 2021 • edited Loading

crlf0710 commented Jul 31, 2021

JohnCSimon commented Sep 6, 2021

This comment has been minimized.

This comment has been minimized.

cjgillot commented Oct 23, 2021

bors commented Oct 23, 2021

bors commented Oct 24, 2021

bors commented Oct 24, 2021

rust-timer commented Oct 24, 2021

tmiasko commented Jul 14, 2021 •

edited

Loading