Skip to content

Conversation

ouz-a
Copy link
Contributor

@ouz-a ouz-a commented May 13, 2022

Follow up work to #96549 #96116 #95857 #95649

This adds validation for Derefer making sure it is always the first projection.

r? rust-lang/mir-opt

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 13, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 13, 2022
@rust-log-analyzer

This comment has been minimized.

@ouz-a ouz-a force-pushed the mini-derefer-generator branch from af54ec1 to ddd58ed Compare May 13, 2022 20:32
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented May 16, 2022

re-rolling as I wrote the change adding a new mir phase (cc @JakobDegen)

r? rust-lang/mir-opt

@oli-obk
Copy link
Contributor

oli-obk commented May 16, 2022

r? @davidtwco

@oli-obk
Copy link
Contributor

oli-obk commented May 16, 2022

@bors try

We should crater this as it adds a new rule to our final MIR output that needs to be upheld or we ICE

@bors
Copy link
Collaborator

bors commented May 16, 2022

⌛ Trying commit 15fb808df52dedb6cd791a19a8eaa3ad88855ba8 with merge 3930b75493eb88cd5431103dbdbc47eb760e9d9b...

@bors
Copy link
Collaborator

bors commented May 16, 2022

☀️ Try build successful - checks-actions
Build commit: 3930b75493eb88cd5431103dbdbc47eb760e9d9b (3930b75493eb88cd5431103dbdbc47eb760e9d9b)

@oli-obk
Copy link
Contributor

oli-obk commented May 17, 2022

@craterbot run mode=build-only

need to run in build mode so that we actually run all mir optimizations

@craterbot
Copy link
Collaborator

👌 Experiment pr-97025 created and queued.
🤖 Automatically detected try build 3930b75493eb88cd5431103dbdbc47eb760e9d9b
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2022
@Mark-Simulacrum
Copy link
Member

@oli-obk just to confirm - debug build is enough to run MIR opts? I don't think crater builds with the release profile today by default.

@oli-obk
Copy link
Contributor

oli-obk commented May 17, 2022

debug build is enough to run MIR opts? I don't think crater builds with the release profile today by default.

yes, it's not important to run any mir opts, it's just important to produce MIR for LLVM

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

LGTM, r=me after resolving other comments

@bors
Copy link
Collaborator

bors commented May 29, 2022

💔 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 May 29, 2022
@rust-log-analyzer

This comment has been minimized.

@tmiasko
Copy link
Contributor

tmiasko commented May 30, 2022

The tests fail on wasm32-unknown-unknown, because this target defaults to -C panic=abort, and the function calls in MIR don't have unwind edges:

-           (*_5) = f() -> [return: bb2, unwind: bb5]; // scope 0 at $DIR/derefer_inline_test.rs:10:9: 10:12
+           (*_5) = f() -> bb2;

We have been generally either ignoring test on this target:
// ignore-wasm32 compiled with panic=abort by default
or compiling with panic=abort:
// compile-flags: -C panic=abort.

@ouz-a
Copy link
Contributor Author

ouz-a commented May 30, 2022

The tests fail on wasm32-unknown-unknown, because this target defaults to -C panic=abort, and the function calls in MIR don't have unwind edges:

-           (*_5) = f() -> [return: bb2, unwind: bb5]; // scope 0 at $DIR/derefer_inline_test.rs:10:9: 10:12
+           (*_5) = f() -> bb2;

We have been generally either ignoring test on this target: // ignore-wasm32 compiled with panic=abort by default or compiling with panic=abort: // compile-flags: -C panic=abort.

Thanks, going to add // ignore-wasm32 compiled with panic=abort by default to the test, I was pretty confused why the test was running fine on my machine and CI but failing on merge...

@ouz-a ouz-a force-pushed the mini-derefer-generator branch from f2325c4 to e71913e Compare May 30, 2022 15:36
@oli-obk
Copy link
Contributor

oli-obk commented May 30, 2022

@bors r=davidtwco

@bors
Copy link
Collaborator

bors commented May 30, 2022

📌 Commit e71913e has been approved by davidtwco

@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 May 30, 2022
@bors
Copy link
Collaborator

bors commented May 30, 2022

⌛ Testing commit e71913e with merge c35035c...

@bors
Copy link
Collaborator

bors commented May 30, 2022

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing c35035c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 30, 2022
@bors bors merged commit c35035c into rust-lang:master May 30, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 30, 2022
@rust-highfive
Copy link
Contributor

📣 Toolstate changed by #97025!

Tested on commit c35035c.
Direct link to PR: #97025

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request May 30, 2022
Tested on commit rust-lang/rust@c35035c.
Direct link to PR: <rust-lang/rust#97025>

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c35035c): comparison url.

Instruction count

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
0.6% 0.8% 3
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvements found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
3.7% 5.4% 4
Improvements 🎉
(primary)
-2.7% -3.2% 2
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) -2.7% -3.2% 2

Cycles

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)
2.4% 2.4% 1
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-2.3% -2.3% 1
All 😿🎉 (primary) 2.4% 2.4% 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 3

  2. number of relevant changes 2 3

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 24, 2022
Pull Derefer before ElaborateDrops

_Follow up work to rust-lang#97025 rust-lang#96549 rust-lang#96116 rust-lang#95887 #95649_

This moves `Derefer` before `ElaborateDrops` and creates a new `Rvalue` called `VirtualRef` that allows us to bypass many constraints for `DerefTemp`.

r? `@oli-obk`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 25, 2022
Pull Derefer before ElaborateDrops

_Follow up work to rust-lang#97025 rust-lang#96549 rust-lang#96116 rust-lang#95887 #95649_

This moves `Derefer` before `ElaborateDrops` and creates a new `Rvalue` called `VirtualRef` that allows us to bypass many constraints for `DerefTemp`.

r? ``@oli-obk``
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 12, 2022
Pull Derefer before ElaborateDrops

_Follow up work to rust-lang#97025 rust-lang#96549 rust-lang#96116 rust-lang#95887 #95649_

This moves `Derefer` before `ElaborateDrops` and creates a new `Rvalue` called `VirtualRef` that allows us to bypass many constraints for `DerefTemp`.

r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 13, 2022
Pull Derefer before ElaborateDrops

_Follow up work to rust-lang#97025 rust-lang#96549 rust-lang#96116 rust-lang#95887 #95649_

This moves `Derefer` before `ElaborateDrops` and creates a new `Rvalue` called `VirtualRef` that allows us to bypass many constraints for `DerefTemp`.

r? `@oli-obk`
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 18, 2022
Pull Derefer before ElaborateDrops

_Follow up work to rust-lang#97025 rust-lang#96549 rust-lang#96116 rust-lang#95887 #95649_

This moves `Derefer` before `ElaborateDrops` and creates a new `Rvalue` called `VirtualRef` that allows us to bypass many constraints for `DerefTemp`.

r? `@oli-obk`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 25, 2022
Simplify some code that depend on Deref

Now that we can assume rust-lang#97025 works, it's safe to expect Deref is always in the first place of projections. With this, I was able to simplify some code that depended on Deref's place in projections. When we are able to move Derefer before `ElaborateDrops` successfully we will be able to optimize more places.

r? `@oli-obk`
@ouz-a ouz-a deleted the mini-derefer-generator branch July 26, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.