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

ARM64 Redundant load/stores for methods that operates/returns structs #35071

Closed
kunalspathak opened this issue Apr 16, 2020 · 5 comments
Closed
Assignees
Labels
arch-arm64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@kunalspathak
Copy link
Member

kunalspathak commented Apr 16, 2020

Part of library code SqlDouble where we are generating code that does redundant stores and loads of x0, x1, x2 and x3 and can be removed for arm64. I tried trimming the repro, but it was not getting the generated code that it would do for the actual scenario. So here is the repro code:

public struct MyType
{
    private double m_value;
    private bool IsNull;

    public MyType(double _value)
    {
        m_value = _value;
        IsNull = false;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static SqlBoolean Check(MyType data1, MyType data2)
    {
        return data1 <= data2;
    }

    public static SqlBoolean operator <=(MyType x, MyType y)
    {
        return (x.IsNull || y.IsNull) ? SqlBoolean.Null : new SqlBoolean(x.m_value <= y.m_value);
    }

    public static SqlBoolean operator >=(MyType x, MyType y)
    {
        return (x.IsNull || y.IsNull) ? SqlBoolean.Null : new SqlBoolean(x.m_value >= y.m_value);
    }
}

public struct SqlBoolean
{
    private bool value;
    public SqlBoolean(bool _value)
    {
        value = _value;
    }
    public static SqlBoolean Null = new SqlBoolean();
}

MyType.Check(new MyType(1.1), new MyType(1.5));

On ARM64, it generates:

G_M35994_IG01:
        A9BD7BFD          stp     fp, lr, [sp,#-48]!
        910003FD          mov     fp, sp
        F90013A0          str     x0, [fp,#32]
        F90017A1          str     x1, [fp,#40]
        F9000BA2          str     x2, [fp,#16]
        F9000FA3          str     x3, [fp,#24]
                                                ;; bbWeight=1    PerfScore 5.50
G_M35994_IG02:
        F94013A0          ldr     x0, [fp,#32]
        F94017A1          ldr     x1, [fp,#40]
        F9400BA2          ldr     x2, [fp,#16]
        F9400FA3          ldr     x3, [fp,#24]
        97FFFFB0          bl      projs.MyType:op_LessThanOrEqual(projs.MyType,projs.MyType):projs.SqlBoolean
                                                ;; bbWeight=1    PerfScore 9.00
G_M35994_IG03:
        A8C37BFD          ldp     fp, lr, [sp],#48
        D65F03C0          ret     lr

category:cq
theme:optimization
skill-level:intermediate
cost:medium

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. 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 Apr 16, 2020
@kunalspathak
Copy link
Member Author

@CarolEidt , @sandreenko - Feel free to tag this to an existing issue if this is already tracked.

@kunalspathak kunalspathak added arch-arm64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Apr 16, 2020
@CarolEidt
Copy link
Contributor

I believe that this would be addressed by #34105

@BruceForstall BruceForstall added this to the Future milestone Apr 20, 2020
@BruceForstall BruceForstall removed the untriaged New issue has not been triaged by the area owner label Apr 20, 2020
@kunalspathak
Copy link
Member Author

@CarolEidt - Now that #34105 is closed, it looks like this case is still not handled and I can see that we still generate redundant load and store.

@BruceForstall BruceForstall added the JitUntriaged CLR JIT issues needing additional triage label Oct 28, 2020
@kunalspathak kunalspathak modified the milestones: Future, 6.0.0 Nov 9, 2020
@kunalspathak kunalspathak added this to Needs Triage in .NET Core CodeGen via automation Nov 9, 2020
@kunalspathak kunalspathak moved this from Needs Triage to Backlog (General) in .NET Core CodeGen Nov 9, 2020
@kunalspathak kunalspathak removed the JitUntriaged CLR JIT issues needing additional triage label Nov 9, 2020
@JulieLeeMSFT JulieLeeMSFT modified the milestones: 6.0.0, Future Mar 23, 2021
@sandreenko sandreenko self-assigned this Apr 12, 2021
@sandreenko sandreenko modified the milestones: Future, 6.0.0 Apr 12, 2021
@sandreenko
Copy link
Contributor

the issue was not fixed in 6.0, I will try to fix it 7.0. #53956 is the same issue but with a simpler repro, so closing this one.

.NET Core CodeGen automation moved this from Backlog (General) to Done Jul 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
Archived in project
Development

No branches or pull requests

6 participants