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

Invalid CSE with field seqs #54102

Closed
jakobbotsch opened this issue Jun 12, 2021 · 5 comments · Fixed by #57282
Closed

Invalid CSE with field seqs #54102

jakobbotsch opened this issue Jun 12, 2021 · 5 comments · Fixed by #57282
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@jakobbotsch
Copy link
Member

// Generated by Fuzzlyn v1.1 on 2021-06-12 11:06:35
// Seed: 8276490119048877745
// Reduced from 962.5 KiB to 0.6 KiB in 00:05:32
// Debug: Outputs 0
// Release: Outputs 1
struct S0
{
    public ushort F1;
    public uint F2;
    public byte F3;
    public int F5;
    public S0(byte f3): this()
    {
        F3 = f3;
    }
}

struct S1
{
    public S0 F0;
    public S0 F4;
    public S1(S0 f0): this()
    {
        F0 = f0;
    }
}

struct S2
{
    public S1 F0;
    public S2(S1 f0): this()
    {
        F0 = f0;
    }
}

public class Program
{
    public static void Main()
    {
        S2 vr0 = new S2(new S1(new S0(1)));
        System.Console.WriteLine(vr0.F0.F0.F3);
        System.Console.WriteLine(vr0.F0.F4.F1);
    }
}

We end up CSE'ing the two field seqs and print 1 twice in release.
cc @dotnet/jit-contrib

@dotnet-issue-labeler
Copy link

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-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jun 12, 2021
@jakobbotsch jakobbotsch added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 12, 2021
@sandreenko sandreenko self-assigned this Jun 12, 2021
@sandreenko sandreenko removed the untriaged New issue has not been triaged by the area owner label Jun 12, 2021
@sandreenko sandreenko added this to the 6.0.0 milestone Jun 12, 2021
@SingleAccretion
Copy link
Contributor

SingleAccretion commented Jun 12, 2021

@sandreenko I've been looking at this a bit, here's a bit simpler reproduction:

public static void Problem()
{
    S2 s2;
    S1 s1;
    S0 s0;

    s1 = default;
    s0.F0_UByte = 1;
    s1.F0_S0 = s0;
    s2.F0_S1 = s1;

    Console.WriteLine(s2.F0_S1.F0_S0.F0_UByte);
}

struct S0
{
    public byte F0_UByte;
}

struct S1
{
    public S0 F0_S0;
    public byte Dummy;
}

struct S2
{
    public S1 F0_S1;
}

With JitNoStructPromotion=1.

And the bad VN:

N002 [000029]   LCL_FLD   V00 loc0         u:2[+0] Fseq[F0_S1, F0_S0, F0_UByte] (last use) => $41 {IntCns 0}

***** BB01, STMT00004(after)
N003 ( 18, 11) [000030] --CXG-------              *  CALL      void   System.Console.WriteLine $VN.Void
N002 (  4,  5) [000029] ------------ arg0 in rcx  \--*  LCL_FLD   ubyte  V00 loc0         u:2[+0] Fseq[F0_S1, F0_S0, F0_UByte] (last use) $41

@sandreenko
Copy link
Contributor

Thanks @jakobbotsch and @SingleAccretion for the repros, I think the issue is similar to #49954, the root cause is in this optimization:

// We're not going to produce a TYP_STRUCT LCL_FLD so we don't need the field sequence.

where we can generate S2 = S1 assignments. The closer we get to 6.0 the more I am thinking about disabling it and accept several kb regressions because my former tries to fix it were unsuccessful.

@jakobbotsch
Copy link
Member Author

Not sure if this one is related:

// Generated by Fuzzlyn v1.2 on 2021-07-06 09:46:44
// Seed: 16635934940619066544
// Reduced from 447.5 KiB to 0.6 KiB in 00:02:24
// Debug: Runs successfully
// Release: Throws 'System.NullReferenceException'
struct S0
{
    public uint F1;
    public byte F3;
    public long F4;
    public uint F5;
    public S0(long f4): this()
    {
        F4 = f4;
    }
}

class C0
{
    public S0 F4;
}

struct S1
{
    public C0 F2;
    public S0 F8;
    public S1(C0 f2, S0 f8) : this()
    {
        F2 = f2;
        F8 = f8;
    }
}

struct S2
{
    public S1 F0;
    public S2(S1 f0): this()
    {
        F0 = f0;
    }
}

public class Program
{
    public static void Main()
    {
        S2 vr0 = new S2(new S1(new C0(), new S0(0)));
        M17(ref vr0.F0.F2.F4.F1);
    }

    static void M17(ref uint arg2)
    {
    }
}

@briansull
Copy link
Contributor

Triage Area: CSE

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
4 participants