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

Potential optimization for is "" #41779

Closed
Youssef1313 opened this issue Sep 2, 2020 · 7 comments · Fixed by #63734
Closed

Potential optimization for is "" #41779

Youssef1313 opened this issue Sep 2, 2020 · 7 comments · Fixed by #63734
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@Youssef1313
Copy link
Member

Youssef1313 commented Sep 2, 2020

Based on the current master in sharplab:

https://sharplab.io/#v2:EYLgtghgzgLgpgJwD4AEDMACFAmDBhAWACgBvYjCrTYAexoBsMBZAChuACs4BjGDADwCU5SmSKUJWAOwCMASygYAREoDcIigF9imoA==

The following:

public bool M(object x)
{
    return x is "";
}

is compiled as:

public bool M(object x)
{
    string text = x as string;
    if (text != null)
    {
        return text == "";
    }
    return false;
}

It's a bit more performant if return text == "" is text.Length == 0.
That would be a special-casing for this, which I'm unsure if it's acceptable to the compiler team or not.

category:cq
theme:basic-cq
skill-level:intermediate
cost:small

@CyrusNajmabadi
Copy link
Member

It's a bit more performant if return text == "" is text.Length == 0.

That feels like a runtime optimziation to make. text == "" is totally idiomatic and should be jittable to whatever the fastest string check is we can muster. @jkotas thoughts? Should this move to jit?

@jkotas
Copy link
Member

jkotas commented Sep 3, 2020

This is a special case of #33338 .

@jkotas jkotas transferred this issue from dotnet/roslyn Sep 3, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Sep 3, 2020
@jkotas
Copy link
Member

jkotas commented Sep 3, 2020

cc @EgorBo

@jkotas jkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 3, 2020
@EgorBo
Copy link
Member

EgorBo commented Sep 3, 2020

Yes, if we could tune the inliner to be able to inline string.Equals (called via == operator) for constant arguments we could achieve the desired behavior and fold redundant null-checks.

// Determines whether two Strings match.
public static bool Equals(string? a, string? b)
{
    if (object.ReferenceEquals(a, b))
    {
        return true;
    }

    if (a is null || b is null || a.Length != b.Length)
    {
        return false;
    }

    return EqualsHelper(a, b);
}

(the idea was to tell the inliner "if you see a get_Length call on a constant string especially in a condition - try harder")

However, I guess redundant object.ReferenceEquals(a, b) and EqualsHelper(a, b); would still be presented in the final codegen. So probably makes sense to intrinsify string.Equals and if its argument is a constant string - emit GT_ARRLEN check.

@EgorBo
Copy link
Member

EgorBo commented Sep 3, 2020

Prototype with an intrinsic: EgorBo@7c4ef9f

it optimizes x == "" or x is "" or string.Equals(x, "") to x != null && x.Length == 0

@JulieLeeMSFT JulieLeeMSFT removed the untriaged New issue has not been triaged by the area owner label Sep 3, 2020
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Sep 3, 2020
@BruceForstall BruceForstall added the JitUntriaged CLR JIT issues needing additional triage label Oct 28, 2020
@BruceForstall BruceForstall removed the JitUntriaged CLR JIT issues needing additional triage label Nov 7, 2020
@JulieLeeMSFT JulieLeeMSFT added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Mar 23, 2021
@EgorBo
Copy link
Member

EgorBo commented Apr 6, 2021

Moving to future
First, we need to be able to import string.Empty as GT_CNS_STR to maximize impact of this.

@EgorBo EgorBo modified the milestones: 6.0.0, Future Apr 6, 2021
@EgorBo EgorBo removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Apr 6, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 13, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 19, 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
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants