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

Do not unify dereferences of shared borrows in GVN #132461

Closed
wants to merge 3 commits into from

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Nov 1, 2024

The pass as written does not differentiate single deref or nested derefs, so I'm gating the entire unification on -Zunsound-mir-opts. This has the unfortunate effect of preventing reading from statics.

Fixes #130853

r? @RalfJung

@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 Nov 1, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 1, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Nov 1, 2024

Some changes occurred in coverage tests.

cc @Zalathar

@DianQK
Copy link
Member

DianQK commented Nov 2, 2024

Do you think it makes sense to invalidate all known Deref when encountering with a dereference assignment or a function call? If so, then r=me.
Context: I'm trying to fix #128299, so I still need the Deref fact.

@RalfJung
Copy link
Member

RalfJung commented Nov 2, 2024

Thanks!
Can you add the example from #130853 as a test?

// CHECK: _0 = Eq({{.*}}, {{.*}});
// CHECK-NOT: _0 = const true;
let y = **x;
unsafe { unknown() };
Copy link
Member

Choose a reason for hiding this comment

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

Here~ @RalfJung

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's drowning in all the other diff noise for tests changing their output.^^

I find mir-opt diffs completely unreadable so I largely skip them...

@@ -0,0 +1,24 @@
// EMIT_MIR_FOR_EACH_PANIC_STRATEGY
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// EMIT_MIR_FOR_EACH_PANIC_STRATEGY
//! Regression test for <https://github.com/rust-lang/rust/issues/130853>
// EMIT_MIR_FOR_EACH_PANIC_STRATEGY

@RalfJung
Copy link
Member

RalfJung commented Nov 2, 2024

Okay, the code diff as well as the new test seem right to me. I have absolutely no idea what to do with the remaining diff.^^

// CHECK: assert(const true,
// CHECK: [[a]] = const 2_u32;
// CHECK-NOT: assert(const true,
// CHECK-NOT: [[a]] = const 2_u32;
Copy link
Member

Choose a reason for hiding this comment

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

Seems odd to have tests asserting that an optimization does not happen. Shouldn't we instead keep the old assertions but comment them out?

Copy link
Contributor

@klensy klensy Nov 5, 2024

Choose a reason for hiding this comment

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

There is COM: prefix for that, i.e. COM: CHECK: assert(const true, but it nice to add fixme for that, to not to forget what it was about.

@RalfJung
Copy link
Member

RalfJung commented Nov 5, 2024

@rustbot author

@rustbot rustbot 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 Nov 5, 2024
@bors
Copy link
Contributor

bors commented Nov 10, 2024

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

@RalfJung
Copy link
Member

Since @cjgillot seems to be busy I made an updated version of this PR: #133474

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 25, 2024
Do not unify dereferences of shared borrows in GVN

Repost of rust-lang#132461, the last commit applies my suggestions.

Fixes rust-lang#130853
@RalfJung
Copy link
Member

Closing in favor of #133474

@RalfJung RalfJung closed this Nov 26, 2024
@cjgillot cjgillot deleted the gvn-noderef branch November 27, 2024 01:17
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 27, 2024
…errors

Do not unify dereferences of shared borrows in GVN

Repost of rust-lang#132461, the last commit applies my suggestions.

Fixes rust-lang#130853
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Nov 28, 2024
Do not unify dereferences of shared borrows in GVN

Repost of rust-lang/rust#132461, the last commit applies my suggestions.

Fixes rust-lang/rust#130853
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

Miscompile in the GVN transform
7 participants