Skip to content

[mir-opt] avoid *& when reading primitive from slice #138936

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

Closed
scottmcm opened this issue Mar 25, 2025 · 3 comments · Fixed by #139327
Closed

[mir-opt] avoid *& when reading primitive from slice #138936

scottmcm opened this issue Mar 25, 2025 · 3 comments · Fixed by #139327
Labels
A-mir-opt Area: MIR optimizations A-mir-opt-GVN Area: MIR opt Global Value Numbering (GVN) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@scottmcm
Copy link
Member

Today, this code https://play.rust-lang.org/?version=nightly&mode=release&edition=2024&gist=09a5eaf64b3be527fef5a0589d6fe377

use std::cmp::Ordering;
pub fn foo(x: [i32; 1], y: [i32; 1]) -> Ordering {
    Ord::cmp(&x[0], &y[0])
}

gives this optimized MIR:

fn foo(_1: [i32; 1], _2: [i32; 1]) -> std::cmp::Ordering {
    debug x => _1;
    debug y => _2;
    let mut _0: std::cmp::Ordering;
    let _3: &i32;
    let _4: &i32;
    scope 1 (inlined std::cmp::impls::<impl Ord for i32>::cmp) {
        let mut _5: i32;
        let mut _6: i32;
    }

    bb0: {
        _3 = &_1[0 of 1];
        _4 = &_2[0 of 1];
        StorageLive(_5);
        _5 = copy (*_3);
        StorageLive(_6);
        _6 = copy (*_4);
        _0 = Cmp(move _5, move _6);
        StorageDead(_6);
        StorageDead(_5);
        return;
    }
}

It's a shame that we need to take a reference there in the MIR instead of just copying out of the place directly, like

    bb0: {
        StorageLive(_5);
        _5 = copy _1[0 of 1];
        StorageLive(_6);
        _6 = copy _2[0 of 1];
        _0 = Cmp(move _5, move _6);
        StorageDead(_6);
        StorageDead(_5);
        return;
    }

@cjgillot, is this something you think GVN could plausibly do?

@scottmcm scottmcm added A-mir-opt Area: MIR optimizations C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 25, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 25, 2025
@dianqk dianqk added the A-mir-opt-GVN Area: MIR opt Global Value Numbering (GVN) label Mar 26, 2025
@dianqk
Copy link
Member

dianqk commented Mar 26, 2025

I think GVN only tracks the value that is a local, then reuse the value (local). Maybe we can do a pre-processing before GVN to put these values into locals: https://play.rust-lang.org/?version=nightly&mode=release&edition=2024&gist=38d1e888c464c4bc190f04d7aa27ecd3.

@cjgillot
Copy link
Contributor

cjgillot commented Apr 3, 2025

I think it can be done with GVN. The hard part is checking that the place still exists and has the same value. I'm surprised ReferencePropagation doesn't do it already. #132527 may be enough to unblock GVN.

@cjgillot
Copy link
Contributor

cjgillot commented Apr 3, 2025

In this case, ReferencePropagation cannot do it, as the index is not known when it runs. GVN could do it. Most of the required code already exists in simplify_aggregate_to_copy and needs to be generalized.

bors added a commit to rust-lang-ci/rust that referenced this issue Apr 3, 2025
Allow GVN to produce places and not just locals.

That may be too big of a hammer, as we may introduce new deref projections (possible UB footgun + probably not good for perf).

r? `@ghost` for perf

Fixes rust-lang#138936
cc `@scottmcm` `@dianqk`
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 4, 2025
Allow GVN to produce places and not just locals.

That may be too big of a hammer, as we may introduce new deref projections (possible UB footgun + probably not good for perf).

r? `@ghost` for perf

Fixes rust-lang#138936
cc `@scottmcm` `@dianqk`
@bors bors closed this as completed in f06e5c1 Apr 9, 2025
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Apr 10, 2025
github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this issue Apr 10, 2025
Allow GVN to produce places and not just locals.

That may be too big of a hammer, as we may introduce new deref projections (possible UB footgun + probably not good for perf).

The second commit opts out of introducing projections that don't have a stable offset, which is probably what we want. Hence no new Deref and no new Index projections.

Fixes rust-lang/rust#138936
cc `@scottmcm` `@dianqk`
github-merge-queue bot pushed a commit to rust-lang/miri that referenced this issue Apr 10, 2025
Allow GVN to produce places and not just locals.

That may be too big of a hammer, as we may introduce new deref projections (possible UB footgun + probably not good for perf).

The second commit opts out of introducing projections that don't have a stable offset, which is probably what we want. Hence no new Deref and no new Index projections.

Fixes rust-lang/rust#138936
cc `@scottmcm` `@dianqk`
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 A-mir-opt-GVN Area: MIR opt Global Value Numbering (GVN) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants