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

Codegen difference: Local vs IL Stack #75206

Closed
deeprobin opened this issue Sep 7, 2022 · 3 comments · Fixed by #79346
Closed

Codegen difference: Local vs IL Stack #75206

deeprobin opened this issue Sep 7, 2022 · 3 comments · Fixed by #79346
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@deeprobin
Copy link
Contributor

Sharplab

public class C {
    public int A(int i) {
        i += 2;
        i += 4;
        return i;
    }
    public int B(int i) {
        var j = i;
        j += 2;
        j += 4;
        return j;
    }
    public int ConsumeA(int i) => A(i);
    public int ConsumeB(int i) => B(i);
}

According to @SingleAccretion, this is because Roslyn prefers to use the IL stack for locals instead of parameters.
So an optimization would make sense for this.

I would argue that there is a real-world case, as I have seen code where no new variable was created but the parameter was modified directly.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 7, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 7, 2022
@ghost
Copy link

ghost commented Sep 7, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Sharplab

public class C {
    public int A(int i) {
        i += 2;
        i += 4;
        return i;
    }
    public int B(int i) {
        var j = i;
        j += 2;
        j += 4;
        return j;
    }
    public int ConsumeA(int i) => A(i);
    public int ConsumeB(int i) => B(i);
}

According to @SingleAccretion, this is because Roslyn prefers to use the IL stack for locals instead of parameters.
So an optimization would make sense for this.

I would argue that there is a real-world case, as I have seen code where no new variable was created but the parameter was modified directly.

Author: deeprobin
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@SingleAccretion SingleAccretion added the help wanted [up-for-grabs] Good issue for external contributors label Sep 7, 2022
@SingleAccretion SingleAccretion added this to the Future milestone Sep 7, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Sep 7, 2022
@SingleAccretion
Copy link
Contributor

The optimization for locals is fairly simple and can be done in global morph (a bit like the SIMD assignment coalescence works right now). The perhaps more interesting case is making this work for indirect (aka field) stores:

*p += 4;
*p += 4;
=>
*p += 8;

This would have the benefit of avoiding relatively expensive memory operations, but would also require more thorough analysis to determine legality.

@AndyAyersMS
Copy link
Member

Forward sub and or VN prop might also be places to consider.

Forward sub is currently blocked by the fact that these sorts of locals have many appearances, but if there are adjacent statements that both def the same local.....

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 11, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 11, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
4 participants