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

[Fuzzlyn] Failure for another complex expression #85611

Closed
tannergooding opened this issue May 1, 2023 · 5 comments · Fixed by #89088
Closed

[Fuzzlyn] Failure for another complex expression #85611

tannergooding opened this issue May 1, 2023 · 5 comments · Fixed by #89088
Assignees
Labels
arch-x64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI bug in-pr There is an active PR which will close this issue when it is merged Priority:3 Work that is nice to have
Milestone

Comments

@tannergooding
Copy link
Member

This is new to .NET 8 and repros in Preview 3

// Generated by Fuzzlyn v1.5 on 2023-05-01 18:27:03
// Run on X64 Windows
// Seed: 13596672517773814950
// Reduced from 193.8 KiB to 1.2 KiB in 00:03:38
// Debug: Outputs 0
// Release: Outputs 255
public struct S0
{
    public byte F3;
    public uint F4;
    public long F5;
    public byte F6;
    public uint F7;
}

public struct S1
{
    public S0 F2;
}

public class Program
{
    public static IRuntime s_rt;
    public static ushort s_1;
    public static S1 s_2;
    public static S1 s_15;
    public static void Main()
    {
        s_rt = new Runtime();
        S0 vr19 = s_2.F2;
        var vr20 = new S1();
        var vr21 = s_2.F2.F4;
        var vr22 = new short[] { 0 };
        M24(vr20, vr19.F3, vr21, vr22, (sbyte)M23(vr19, 0), 1);
    }

    public static ulong M23(S0 arg1, ushort arg2)
    {
        var vr3 = arg1.F3--;
        var vr4 = arg1.F7;
        var vr6 = new short[] { 0 };
        var vr7 = (sbyte)s_1;
        Program.M24(s_15, vr3, vr4, vr6, vr7, arg2);
        return s_1;
    }

    public static void M24(S1 argThis, byte arg0, uint arg1, short[] arg2, sbyte arg3, ushort arg4)
    {
        s_rt.WriteLine(arg0);
    }
}

public interface IRuntime
{
    void WriteLine<T>(T value);
}

public class Runtime : IRuntime
{
    public void WriteLine<T>(T value) => System.Console.WriteLine(value);
}
@tannergooding tannergooding added bug arch-x64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels May 1, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 1, 2023
@ghost
Copy link

ghost commented May 1, 2023

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

Issue Details

This is new to .NET 8 and repros in Preview 3

// Generated by Fuzzlyn v1.5 on 2023-05-01 18:27:03
// Run on X64 Windows
// Seed: 13596672517773814950
// Reduced from 193.8 KiB to 1.2 KiB in 00:03:38
// Debug: Outputs 0
// Release: Outputs 255
public struct S0
{
    public byte F3;
    public uint F4;
    public long F5;
    public byte F6;
    public uint F7;
}

public struct S1
{
    public S0 F2;
}

public class Program
{
    public static IRuntime s_rt;
    public static ushort s_1;
    public static S1 s_2;
    public static S1 s_15;
    public static void Main()
    {
        s_rt = new Runtime();
        S0 vr19 = s_2.F2;
        var vr20 = new S1();
        var vr21 = s_2.F2.F4;
        var vr22 = new short[] { 0 };
        M24(vr20, vr19.F3, vr21, vr22, (sbyte)M23(vr19, 0), 1);
    }

    public static ulong M23(S0 arg1, ushort arg2)
    {
        var vr3 = arg1.F3--;
        var vr4 = arg1.F7;
        var vr6 = new short[] { 0 };
        var vr7 = (sbyte)s_1;
        Program.M24(s_15, vr3, vr4, vr6, vr7, arg2);
        return s_1;
    }

    public static void M24(S1 argThis, byte arg0, uint arg1, short[] arg2, sbyte arg3, ushort arg4)
    {
        s_rt.WriteLine(arg0);
    }
}

public interface IRuntime
{
    void WriteLine<T>(T value);
}

public class Runtime : IRuntime
{
    public void WriteLine<T>(T value) => System.Console.WriteLine(value);
}
Author: tannergooding
Assignees: -
Labels:

bug, arch-x64, area-CodeGen-coreclr

Milestone: -

@JulieLeeMSFT JulieLeeMSFT removed the untriaged New issue has not been triaged by the area owner label May 1, 2023
@JulieLeeMSFT JulieLeeMSFT added this to the 8.0.0 milestone May 1, 2023
@TIHan
Copy link
Contributor

TIHan commented May 1, 2023

Quickly tested this, not related to cac06a5

@jakobbotsch
Copy link
Member

jakobbotsch commented May 1, 2023

@jakobbotsch
Copy link
Member

Actually this looks like db717e3.

@jakobbotsch
Copy link
Member

This is a variant on #81049, here we have call args morphing reordering a LCL_FLD on a newly address exposed local with the call that elided the copy.

@jakobbotsch jakobbotsch added Priority:2 Work that is important, but not critical for the release Priority:3 Work that is nice to have and removed Priority:2 Work that is important, but not critical for the release labels Jun 20, 2023
jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Jul 18, 2023
Last-use copy omission allows the JIT to avoid creating copies of struct
values that are passed implicitly by-ref when the value is a last-use of
a local. When we do this we effectively address expose the
local for the lifetime of the call, so we mark the local as address
exposed as part of doing so.

However, there is a problem where morph may reorder two uses of a such a
local and break the last use information. For example, consider the
following case:

```
[000015] --CXG------      ▌  CALL      void   Program:Foo(int,int)
[000010] ----------- arg0 ├──▌  LCL_FLD   int    V00 loc0         [+0]
[000012] --CXG------ arg1 └──▌  CALL      int    Program:Bar(S):int
[000011] ----------- arg0    └──▌  LCL_VAR   struct<S, 32> V00 loc0          (last use)
```

V00 is used both at [000010] and at [000011], the latter as a last use.
Since we only address expose V00 when we get to [000011] we do not mark
[000010] with GTF_GLOB_REF; the net effect is the following call args
morphing, where we have reordered the two occurrences illegally:

```
[000015] --CXG+-----             ▌  CALL      void   Program:Foo(int,int)
[000037] DACXG------ arg1 setup  ├──▌  STORE_LCL_VAR int    V04 tmp3
[000012] --CXG+-----             │  └──▌  CALL      int    Program:Bar(S):int
[000011] -----+----- arg0 in rcx │     └──▌  LCL_ADDR  long   V00 loc0         [+0]
[000038] ----------- arg1 in rdx ├──▌  LCL_VAR   int    V04 tmp3
[000010] -----+----- arg0 in rcx └──▌  LCL_FLD   int   (AX) V00 loc0         [+0]
```

This change fixes the problem by running a separate pass over the IR
before morph to identify and address expose the candidates for last-use
copy omission. The net result is then the following correct IR:

```
[000015] --CXG+-----             ▌  CALL      void   Runtime_85611:Foo(int,int)
[000039] DA--G------ arg0 setup  ├──▌  STORE_LCL_VAR int    V05 tmp4
[000010] ----G+-----             │  └──▌  LCL_FLD   int   (AX) V00 loc0         [+0]
[000037] DACXG------ arg1 setup  ├──▌  STORE_LCL_VAR int    V04 tmp3
[000012] --CXG+-----             │  └──▌  CALL      int    Runtime_85611:Bar(Runtime_85611+S):int
[000011] ----G+----- arg0 in rcx │     └──▌  LCL_ADDR  long   V00 loc0         [+0]
[000038] ----------- arg1 in rdx ├──▌  LCL_VAR   int    V04 tmp3
[000040] ----------- arg0 in rcx └──▌  LCL_VAR   int    V05 tmp4
```

The pass has some TP impact but it is mitigated since we only need to
visit statements with GTF_CALL.

Fix dotnet#85611
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 18, 2023
jakobbotsch added a commit that referenced this issue Jul 19, 2023
…B_REF (#89088)

Last-use copy omission allows the JIT to avoid creating copies of struct
values that are passed implicitly by-ref when the value is a last-use of
a local. When we do this we effectively address expose the
local for the lifetime of the call, so we mark the local as address
exposed as part of doing so.

However, there is a problem where morph may reorder two uses of a such a
local and break the last use information. For example, consider the
following case:

```
[000015] --CXG------      ▌  CALL      void   Program:Foo(int,int)
[000010] ----------- arg0 ├──▌  LCL_FLD   int    V00 loc0         [+0]
[000012] --CXG------ arg1 └──▌  CALL      int    Program:Bar(S):int
[000011] ----------- arg0    └──▌  LCL_VAR   struct<S, 32> V00 loc0          (last use)
```

V00 is used both at [000010] and at [000011], the latter as a last use.
Since we only address expose V00 when we get to [000011] we do not mark
[000010] with GTF_GLOB_REF; the net effect is the following call args
morphing, where we have reordered the two occurrences illegally:

```
[000015] --CXG+-----             ▌  CALL      void   Program:Foo(int,int)
[000037] DACXG------ arg1 setup  ├──▌  STORE_LCL_VAR int    V04 tmp3
[000012] --CXG+-----             │  └──▌  CALL      int    Program:Bar(S):int
[000011] -----+----- arg0 in rcx │     └──▌  LCL_ADDR  long   V00 loc0         [+0]
[000038] ----------- arg1 in rdx ├──▌  LCL_VAR   int    V04 tmp3
[000010] -----+----- arg0 in rcx └──▌  LCL_FLD   int   (AX) V00 loc0         [+0]
```

This change fixes the problem by running a separate pass over the IR
before morph to identify and address expose the candidates for last-use
copy omission. The net result is then the following correct IR:

```
[000015] --CXG+-----             ▌  CALL      void   Runtime_85611:Foo(int,int)
[000039] DA--G------ arg0 setup  ├──▌  STORE_LCL_VAR int    V05 tmp4
[000010] ----G+-----             │  └──▌  LCL_FLD   int   (AX) V00 loc0         [+0]
[000037] DACXG------ arg1 setup  ├──▌  STORE_LCL_VAR int    V04 tmp3
[000012] --CXG+-----             │  └──▌  CALL      int    Runtime_85611:Bar(Runtime_85611+S):int
[000011] ----G+----- arg0 in rcx │     └──▌  LCL_ADDR  long   V00 loc0         [+0]
[000038] ----------- arg1 in rdx ├──▌  LCL_VAR   int    V04 tmp3
[000040] ----------- arg0 in rcx └──▌  LCL_VAR   int    V05 tmp4
```

The pass has some TP impact but it is mitigated since we only need to
visit statements with GTF_CALL and since we can use the locals tree list
to prune which statements we visit.

Fix #85611
@ghost ghost locked as resolved and limited conversation to collaborators Aug 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-x64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI bug in-pr There is an active PR which will close this issue when it is merged Priority:3 Work that is nice to have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants