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

Add validation layer for Derefer #97025

Merged
merged 1 commit into from
May 30, 2022
Merged

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.

@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
Contributor

bors commented May 16, 2022

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

@bors
Copy link
Contributor

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
Contributor

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...

@oli-obk
Copy link
Contributor

oli-obk commented May 30, 2022

@bors r=davidtwco

@bors
Copy link
Contributor

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
Contributor

bors commented May 30, 2022

⌛ Testing commit e71913e with merge c35035c...

@bors
Copy link
Contributor

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
Collaborator

📣 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.