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 new Deinit statement #95125

Merged
merged 7 commits into from
Apr 11, 2022
Merged

Conversation

JakobDegen
Copy link
Contributor

@JakobDegen JakobDegen commented Mar 19, 2022

This rvalue replaces SetDiscriminant for ADTs. This PR is an alternative to #94590 , which only specifies that the behavior of SetDiscriminant is the same as what this rvalue would do. The motivation for this change are discussed in that PR and on Zulip

r? @oli-obk

@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

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

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@JakobDegen
Copy link
Contributor Author

Bot doesn't pick it up, so cc @bjorn3 for the cranelift change and... probably someone for clippy, but not sure

@rust-log-analyzer

This comment has been minimized.

@bjorn3
Copy link
Member

bjorn3 commented Mar 20, 2022

cg_clif changes looks fine to me.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 20, 2022

@bors try @rust-timer queue

Will review next week, let's see what perf says 'till then

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 20, 2022
@bors
Copy link
Contributor

bors commented Mar 20, 2022

⌛ Trying commit 7236347a2324c8d571138f81dafd8b150f17eb5b with merge a920b8a92d5e86353acdc0c28bbcaa945ce3b565...

@bors
Copy link
Contributor

bors commented Mar 20, 2022

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

@rust-timer
Copy link
Collaborator

Queued a920b8a92d5e86353acdc0c28bbcaa945ce3b565 with parent 499d4a5, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a920b8a92d5e86353acdc0c28bbcaa945ce3b565): comparison url.

Summary: This benchmark run did not return any relevant results. 7 results were found to be statistically significant but too small to be relevant.

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 20, 2022
@bjorn3
Copy link
Member

bjorn3 commented Mar 20, 2022

This seems to be a tiny slowdown overall.

@JakobDegen
Copy link
Contributor Author

Yeah, that might be the result of an additional variant on Rvalue which is matched on quite a bit

@JakobDegen
Copy link
Contributor Author

The new commits include most of the changes that were also made in #94590 . The only outstanding concern there was Nagisa bringing up that an issue should be opened for the disabled tests - I can do that once either PR merges

@JakobDegen JakobDegen marked this pull request as ready for review March 21, 2022 02:13
compiler/rustc_borrowck/src/invalidation.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved
@JakobDegen
Copy link
Contributor Author

I had meant to add the validator checks but forgot about it - also addressed the other two concerns

@oli-obk
Copy link
Contributor

oli-obk commented Apr 11, 2022

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Apr 11, 2022

📌 Commit 2f03767 has been approved by oli-obk

@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 Apr 11, 2022
@bors
Copy link
Contributor

bors commented Apr 11, 2022

⌛ Testing commit 2f03767 with merge 625e4dd...

@bors
Copy link
Contributor

bors commented Apr 11, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 11, 2022
@bors bors merged commit 625e4dd into rust-lang:master Apr 11, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 11, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (625e4dd): comparison url.

Summary:

  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 27 39 4 17 31
mean2 0.5% 1.4% -0.9% -1.0% 0.3%
max 1.6% 4.1% -1.4% -2.2% 1.6%

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. number of relevant changes

  2. the arithmetic mean of the percent change

@rustbot rustbot added the perf-regression Performance regression. label Apr 11, 2022
@JakobDegen JakobDegen deleted the uninit-variant-rvalue branch April 11, 2022 20:33
@JakobDegen
Copy link
Contributor Author

A small perf regression is to be expected here I think. MIR contains more statements now, and even though we don't codegen the new ones, we still have to process them in rustc. The cost of this could definitely be reduced by optimizations as Oli already mentioned, but I don't think this particular case is any more interesting than all the other examples of optimizations we could do but don't yet.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 12, 2022

I ran cachegrind on some of the regressions, adn all of them seem to be in metadata de/serialization except for cranelift, which looks like LLVM noise. Considering that this is groundwork for fixing bugs in our dataflow analyses, which directly impact MIR opt sounness, I think we have to take this hit

@rustbot label: +perf-regression-triaged


/// Mark the entire referenced range as uninitalized
pub fn write_uninit(&mut self) {
self.alloc.mark_init(self.range, false);
Copy link
Member

@RalfJung RalfJung Apr 17, 2022

Choose a reason for hiding this comment

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

Uh-oh, making mark_init public was a mistake. (Not sure when that happened, that was before this PR.)

This call is wrong, since it forgets to also clear the relocations in that range. Some other uses of mark_init are also wrong. I am working on a fix.

(To be clear, this is not your fault. We did bad API design here.)

celinval added a commit to celinval/kani-dev that referenced this pull request May 10, 2022
We codegen an assignment to non-det value per documentation. See more
information here:
 - rust-lang/rust#95125
celinval added a commit to celinval/kani-dev that referenced this pull request May 11, 2022
We codegen an assignment to non-det value per documentation. See more
information here:
 - rust-lang/rust#95125
celinval added a commit to model-checking/kani that referenced this pull request May 12, 2022
* Update rust toolchain to nightly-2022-05-03

This compiles but regression is failing due to unimplemented statement.

* Handle change to Box<T> structure

Box<T> now users NonNull<T> instead of raw pointer.

* Handle new statement kind Deinit

We codegen an assignment to non-det value per documentation. See more
information here:
 - rust-lang/rust#95125

* Fix discriminant computation

After the merge, the previous wrapping sub logic was triggering a panic
due to u128 -> i64 conversion. There were also other overflow issues
when trying to convert the `niche_value` to unsigned.

For now, I'm disabling the coversion check which I believe is too
strict. We should consider implementing a more flexible check later that
can be controlled by the user without affecting the internal compiler
codegen.

* Address PR comments:

 - Improve comments.
 - Remove wrong cast to i64.
 - Fix statement location.
 - Create util function to create unsigned type.
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. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.