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

Implement simple CopyPropagation based on SSA analysis #106908

Merged
merged 12 commits into from
Jan 29, 2023

Conversation

cjgillot
Copy link
Contributor

This PR extracts the "copy propagation" logic from #106285.

MIR may produce chains of assignment between locals, like _x = move? _y.
This PR attempts to remove such chains by unifying locals.

The current implementation is a bit overzealous in turning moves into copies, and in removing storage statements.

@rustbot
Copy link
Collaborator

rustbot commented Jan 15, 2023

r? @TaKO8Ki

(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 15, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 15, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@oli-obk
Copy link
Contributor

oli-obk commented Jan 16, 2023

r? @oli-obk

@rustbot rustbot assigned oli-obk and unassigned TaKO8Ki Jan 16, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Jan 17, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jan 17, 2023

📌 Commit c1a924547eaecff4bc7af3c0de848429a7ac78f1 has been approved by oli-obk

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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2023
if matches!(body.local_kind(local), LocalKind::Arg) {
this.assignments[local] = Set1::One(LocationExtended::Arg);
}
if borrowed.contains(local) && !decl.ty.is_freeze(tcx, param_env) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The freeze check is not enough here, we should be bailing out on all references. The pass currently changes the bahavior of this code:

#![feature(custom_mir, core_intrinsics)]
#![allow(unused_assignments)]
extern crate core;
use core::intrinsics::mir::*;

#[custom_mir(dialect = "analysis", phase = "post-cleanup")]
fn f(c: bool) -> bool { mir!({
    let a = c;
    let p = core::ptr::addr_of!(a);
    let p2 = core::ptr::addr_of_mut!(*p);
    *p2 = false;
    RET = c;
    Return()
})}

fn main() {
    assert_eq!(true, f(true));
}

This is Miri UB today, but we should not depend on that on stable, especially because there is a real possibility that this may be allowed in the future.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 18, 2023

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 18, 2023
@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 19, 2023
@JakobDegen
Copy link
Contributor

Sorry, should have thought of this before too. Banning raw pointers isn't enough, references cause problems too. Your pass changes the behavior of this code:

#![feature(custom_mir, core_intrinsics)]
#![allow(unused_assignments)]
extern crate core;
use core::intrinsics::mir::*;

fn cmp_ref(a: &u8, b: &u8) -> bool {
    std::ptr::eq(a as *const u8, b as *const u8)
}

#[custom_mir(dialect = "analysis", phase = "post-cleanup")]
fn f() -> bool { mir!(
    {
        let a = 5_u8;
        let r1 = &a;
        let b = a;
        let r2 = &b;
        Call(RET, next, cmp_ref(r1, r2))
    }
    next = {
        Return()
    }
)}

fn main() {
    assert!(!f());
}

Ideally this would actually be allowed, but we don't currently know how to do that, so need to be conservative in the meantime. See rust-lang/unsafe-code-guidelines#206 for discussion

@JakobDegen
Copy link
Contributor

Ah, not the change I was expecting, but very nice. Technically this requires a T-opsem FCP these days (since it makes slightly more assumptions about reference immutability than we do already), but that team doesn't really exist yet, so should be fine to land

@cjgillot cjgillot added the A-mir-opt Area: MIR optimizations label Jan 21, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Jan 23, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jan 23, 2023

📌 Commit cbfb98fb8bf1772e059df697c8d15a71b79256d3 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 23, 2023
@cjgillot
Copy link
Contributor Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jan 27, 2023

📌 Commit 263da25 has been approved by oli-obk

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 27, 2023
@bors
Copy link
Contributor

bors commented Jan 29, 2023

⌛ Testing commit 263da25 with merge 2a4b00b...

@bors
Copy link
Contributor

bors commented Jan 29, 2023

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

1 similar comment
@bors
Copy link
Contributor

bors commented Jan 29, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 29, 2023
@bors bors merged commit 2a4b00b into rust-lang:master Jan 29, 2023
@rustbot rustbot added this to the 1.69.0 milestone Jan 29, 2023
@cjgillot cjgillot deleted the copyprop-ssa branch January 29, 2023 17:15
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2a4b00b): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.8% [3.8%, 3.8%] 1
Regressions ❌
(secondary)
2.8% [2.8%, 2.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.8% [3.8%, 3.8%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 30, 2023
Reimplement NormalizeArrayLen based on SsaLocals

Based on rust-lang#106908
Fixes rust-lang#105929

Only the last commit "Reimplement NormalizeArrayLen" is relevant.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 2, 2023
Remove both StorageLive and StorageDead in CopyProp.

Fixes rust-lang#107511

rust-lang#106908 removed StorageDead without the accompanying StorageLive. In loops, execution would see repeated StorageLive, without any StorageDead, which is UB.

So when removing storage statements, we have to remove both StorageLive and StorageDead.

~I also added a MIR validation pass for StorageLive. It may be a bit overzealous.~
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 2, 2023
Remove both StorageLive and StorageDead in CopyProp.

Fixes rust-lang#107511

rust-lang#106908 removed StorageDead without the accompanying StorageLive. In loops, execution would see repeated StorageLive, without any StorageDead, which is UB.

So when removing storage statements, we have to remove both StorageLive and StorageDead.

~I also added a MIR validation pass for StorageLive. It may be a bit overzealous.~
@tmiasko tmiasko mentioned this pull request Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt Area: MIR optimizations 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.

8 participants