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

Fold IND(LDELEMA_REF(obj, index, type)) to INDEX(obj, index) #65836

Closed
wants to merge 4 commits into from

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Feb 24, 2022

Closes #65789 let's see what diffs think

public sealed class C
{
    C[] _array;

    C Test1() => Volatile.Read(ref _array[3]);

    void Test2()
    {
        ref C c = ref _array[0];
        c = null;
    }
}

Codegen diff: https://www.diffchecker.com/VQ8cweA8

@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 Feb 24, 2022
@ghost ghost assigned EgorBo Feb 24, 2022
@ghost
Copy link

ghost commented Feb 24, 2022

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

Issue Details

Closes #65789 let's see what diffs think

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo
Copy link
Member Author

EgorBo commented Feb 24, 2022

hm... no diffs, @stephentoub did you expect it to improve some already existing codepath or a planned one only?

@stephentoub
Copy link
Member

Yes, e.g.

return Volatile.Read(ref builder._delta[offset]) ?? matcher.CreateNewTransition(currentState, minterm, offset);

@EgorBo
Copy link
Member Author

EgorBo commented Feb 24, 2022

Ah, I see. Unfortunately that case is not handled by "Forward Substitution" pass which is quite conservative at the moment and I believe it explains zero diffs in general, so instead of IND(LDELEMA_REF) I see ASG(LDELEMA_REF)

I'm going to close it and, instead, try to make all CastHelpers methods inlineable for hot paths so e.g. ldelemaRef would be inlineable - it should improve perf for that case

@EgorBo EgorBo closed this Feb 24, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unnecessary LdelemaRef call not elided with Volatile.Read
3 participants