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

Reimplement NormalizeArrayLen based on SsaLocals #107172

Merged
merged 2 commits into from
Jan 30, 2023
Merged

Conversation

cjgillot
Copy link
Contributor

Based on #106908
Fixes #105929

Only the last commit "Reimplement NormalizeArrayLen" is relevant.

@rustbot
Copy link
Collaborator

rustbot commented Jan 21, 2023

r? @nagisa

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 21, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 21, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

The last commit seems reasonable to me!

r=me (with the issue link removed and the prerequisite PR landed)


pub struct NormalizeArrayLen;

impl<'tcx> MirPass<'tcx> for NormalizeArrayLen {
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
// See #105929
sess.mir_opt_level() >= 4 && sess.opts.unstable_opts.unsound_mir_opts
sess.mir_opt_level() >= 3
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t the comment above be removed at this point?

Comment on lines +56 to +57
&& let Some(cast_ty) = cast_ty.builtin_deref(true)
&& let ty::Slice(..) = cast_ty.ty.kind()
Copy link
Member

Choose a reason for hiding this comment

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

(This is just me trying to understand code rather than suggesting any particular change to be made – you can largely ignore this.) Is this check necessary at all? I guess it is a nice defense-in-depth measure, but what other things an unsizing cast could be to? A dynamic object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, Unsize has to be interpreted based on both input and output types. We only want &[T; _] -> &[T]. The other possible cases are &T -> &dyn Trait, &dyn Trait1 -> &dyn Trait2, Struct<[T; _]> -> Struct<[T]>.

@bors
Copy link
Contributor

bors commented Jan 26, 2023

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

@cjgillot cjgillot 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 Jan 26, 2023
@cjgillot
Copy link
Contributor Author

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Jan 29, 2023

📌 Commit b456307 has been approved by nagisa

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 29, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#107125 (Add and use expect methods to hir.)
 - rust-lang#107172 (Reimplement NormalizeArrayLen based on SsaLocals)
 - rust-lang#107177 (Keep all theme-updating logic together)
 - rust-lang#107424 (Make Vec::clone_from and slice::clone_into share the same code)
 - rust-lang#107455 (use a more descriptive name)
 - rust-lang#107465 (`has_allow_dead_code_or_lang_attr` micro refactor)
 - rust-lang#107469 (Change turbofish context link to an archive link)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit db97749 into rust-lang:master Jan 30, 2023
@rustbot rustbot added this to the 1.69.0 milestone Jan 30, 2023
@cjgillot cjgillot deleted the no-nal branch January 30, 2023 20:15
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. 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.

NormalizeArrayLen MIR opt unsound due to missed writes
4 participants