JIT: Insertion of retbufs relies on aggressive importer spilling to not reorder calls

[jakobbotsch]

In #70932 I tried to remove some (very) aggressive call argument spilling that I believe should be unnecessary. With that change the JIT reorders the calls to M53 and M55 in the following program when optimizing:

// Generated by Fuzzlyn v1.5 on 2022-07-16 17:53:24
// Run on X64 Windows
// Seed: 14454118745605583094
// Reduced from 689.5 KiB to 2.0 KiB in 00:06:03
// Debug: Prints 0 line(s)
// Release: Prints 1 line(s)
public struct S0
{
    public int F0;
    public uint F7;
    public uint F8;
    public S0(uint f8): this()
    {
        F8 = f8;
    }
}

public struct S1
{
    public S0 F0;
    public long F2;
    public bool F5;
    public S1(S0 f0): this()
    {
        F0 = f0;
    }
}

public class C0
{
    public ulong F1;
    public C0(ulong f1, sbyte f2, sbyte f3, short f4, sbyte f5, bool f6, byte f7, sbyte f8)
    {
    }

    public S2 M50(ulong arg0)
    {
        return new S2(0, false, 0, 0, 1, 0, new S0(0), true, 1, new C0(0, 0, 0, 0, 0, true, 0, 0));
    }
}

public struct S2
{
    public bool F7;
    public C0 F9;
    public S2(long f0, bool f1, long f2, byte f3, short f4, byte f5, S0 f6, bool f7, byte f8, C0 f9): this()
    {
    }
}

public struct S3
{
    public S1 F0;
}

public class Program
{
    public static IRuntime s_rt;
    public static S2 s_5;
    public static S2[] s_13;
    public static S2 s_19;
    public static S1 s_29 = new S1(new S0(1));
    public static void Main()
    {
        s_rt = new Runtime();
        S1 vr2 = default(S1);
        var vr5 = new S3();
        M53(vr5) = M55(s_29, ref vr2).M50(s_5.F9.F1);
    }

    public static C0 M55(S1 argThis, ref S1 arg0)
    {
        if (!(arg0.F0.F7 == argThis.F0.F8))
        {
            arg0.F5 = s_5.F7;
        }
        else
        {
            return new C0(7660146447784381526UL, 1, 1, -884, 92, true, 14, 10);
        }

        s_rt.WriteLine("c_871", arg0.F0.F0);
        return new C0(6942104764844716959UL, -2, 1, -31945, -40, true, 214, 1);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static ref S2 M53(S3 argThis)
    {
        ulong vr12 = s_13[0].F9.F1;
        long var0 = argThis.F0.F2--;
        return ref s_19;
    }
}

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

public class Runtime : IRuntime
{
    public void WriteLine<T>(string site, T value) => System.Console.WriteLine(value);
}

We end up with

STMT00005 ( 0x01A[E-] ... ??? )
               [000049] S-CXG------                         ▌  CALL nullcheck void   C0.M50
               [000037] --CXG------ this                    ├──▌  CALL      ref    Program.M55
               [000029] ---XG------ arg0                    │  ├──▌  OBJ       struct<S1, 32>
               [000034] --CXG------                         │  │  └──▌  COMMA     byref 
               [000033] H-CXG------                         │  │     ├──▌  CALL help long   HELPER.CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
               [000031] ----------- arg0                    │  │     │  ├──▌  CNS_INT   long   0x7ffc75adf900
               [000032] ----------- arg1                    │  │     │  └──▌  CNS_INT   int    9
               [000028] ----G------                         │  │     └──▌  ADD       byref 
               [000026] ----G------                         │  │        ├──▌  FIELD     ref    s_29
               [000027] -----------                         │  │        └──▌  CNS_INT   long   8 Fseq[s_29]
               [000036] ----------- arg1                    │  └──▌  ADDR      byref 
               [000035] -------N---                         │     └──▌  LCL_VAR   struct<S1, 32> V00 loc0         
               [000023] --C-G------ retbuf                  ├──▌  CALL      byref  Program.M53
               [000025] n---------- arg0                    │  └──▌  OBJ       struct<S3, 32>
               [000024] -----------                         │     └──▌  ADDR      byref 
               [000022] -------N---                         │        └──▌  LCL_VAR   struct<S3, 32> V01 loc1         
               [000048] --CXG------ arg2                    └──▌  FIELD     long   F1
               [000047] --CXG------                            └──▌  FIELD     ref    F9
               [000046] --CXG------                               └──▌  COMMA     byref 
               [000045] H-CXG------                                  ├──▌  CALL help long   HELPER.CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
               [000043] ----------- arg0                             │  ├──▌  CNS_INT   long   0x7ffc75adf900
               [000044] ----------- arg1                             │  └──▌  CNS_INT   int    9
               [000041] -----------                                  └──▌  ADD       byref 
               [000039] #----------                                     ├──▌  IND       ref   
               [000038] I----------                                     │  └──▌  CNS_INT(h) long   0x1fbbc801f20 static box ptr
               [000040] -----------                                     └──▌  CNS_INT   long   8 Fseq[s_5]

The JIT probably makes a wrong assumption about when the retbuf will be evaluated here.

category:cq
theme:importer
skill-level:intermediate
cost:medium
impact:small

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

Issue Details

---

In #70932 I tried to remove some (very) aggressive call argument spilling that I believe should be unnecessary. With that change the JIT reorders the calls to M53 and M55 in the following program when optimizing:

// Generated by Fuzzlyn v1.5 on 2022-07-16 17:53:24
// Run on X64 Windows
// Seed: 14454118745605583094
// Reduced from 689.5 KiB to 2.0 KiB in 00:06:03
// Debug: Prints 0 line(s)
// Release: Prints 1 line(s)
public struct S0
{
    public int F0;
    public uint F7;
    public uint F8;
    public S0(uint f8): this()
    {
        F8 = f8;
    }
}

public struct S1
{
    public S0 F0;
    public long F2;
    public bool F5;
    public S1(S0 f0): this()
    {
        F0 = f0;
    }
}

public class C0
{
    public ulong F1;
    public C0(ulong f1, sbyte f2, sbyte f3, short f4, sbyte f5, bool f6, byte f7, sbyte f8)
    {
    }

    public S2 M50(ulong arg0)
    {
        return new S2(0, false, 0, 0, 1, 0, new S0(0), true, 1, new C0(0, 0, 0, 0, 0, true, 0, 0));
    }
}

public struct S2
{
    public bool F7;
    public C0 F9;
    public S2(long f0, bool f1, long f2, byte f3, short f4, byte f5, S0 f6, bool f7, byte f8, C0 f9): this()
    {
    }
}

public struct S3
{
    public S1 F0;
}

public class Program
{
    public static IRuntime s_rt;
    public static S2 s_5;
    public static S2[] s_13;
    public static S2 s_19;
    public static S1 s_29 = new S1(new S0(1));
    public static void Main()
    {
        s_rt = new Runtime();
        S1 vr2 = default(S1);
        var vr5 = new S3();
        M53(vr5) = M55(s_29, ref vr2).M50(s_5.F9.F1);
    }

    public static C0 M55(S1 argThis, ref S1 arg0)
    {
        if (!(arg0.F0.F7 == argThis.F0.F8))
        {
            arg0.F5 = s_5.F7;
        }
        else
        {
            return new C0(7660146447784381526UL, 1, 1, -884, 92, true, 14, 10);
        }

        s_rt.WriteLine("c_871", arg0.F0.F0);
        return new C0(6942104764844716959UL, -2, 1, -31945, -40, true, 214, 1);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static ref S2 M53(S3 argThis)
    {
        ulong vr12 = s_13[0].F9.F1;
        long var0 = argThis.F0.F2--;
        return ref s_19;
    }
}

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

public class Runtime : IRuntime
{
    public void WriteLine<T>(string site, T value) => System.Console.WriteLine(value);
}

We end up with

STMT00005 ( 0x01A[E-] ... ??? )
               [000049] S-CXG------                         ▌  CALL nullcheck void   C0.M50
               [000037] --CXG------ this                    ├──▌  CALL      ref    Program.M55
               [000029] ---XG------ arg0                    │  ├──▌  OBJ       struct<S1, 32>
               [000034] --CXG------                         │  │  └──▌  COMMA     byref 
               [000033] H-CXG------                         │  │     ├──▌  CALL help long   HELPER.CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
               [000031] ----------- arg0                    │  │     │  ├──▌  CNS_INT   long   0x7ffc75adf900
               [000032] ----------- arg1                    │  │     │  └──▌  CNS_INT   int    9
               [000028] ----G------                         │  │     └──▌  ADD       byref 
               [000026] ----G------                         │  │        ├──▌  FIELD     ref    s_29
               [000027] -----------                         │  │        └──▌  CNS_INT   long   8 Fseq[s_29]
               [000036] ----------- arg1                    │  └──▌  ADDR      byref 
               [000035] -------N---                         │     └──▌  LCL_VAR   struct<S1, 32> V00 loc0         
               [000023] --C-G------ retbuf                  ├──▌  CALL      byref  Program.M53
               [000025] n---------- arg0                    │  └──▌  OBJ       struct<S3, 32>
               [000024] -----------                         │     └──▌  ADDR      byref 
               [000022] -------N---                         │        └──▌  LCL_VAR   struct<S3, 32> V01 loc1         
               [000048] --CXG------ arg2                    └──▌  FIELD     long   F1
               [000047] --CXG------                            └──▌  FIELD     ref    F9
               [000046] --CXG------                               └──▌  COMMA     byref 
               [000045] H-CXG------                                  ├──▌  CALL help long   HELPER.CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
               [000043] ----------- arg0                             │  ├──▌  CNS_INT   long   0x7ffc75adf900
               [000044] ----------- arg1                             │  └──▌  CNS_INT   int    9
               [000041] -----------                                  └──▌  ADD       byref 
               [000039] #----------                                     ├──▌  IND       ref   
               [000038] I----------                                     │  └──▌  CNS_INT(h) long   0x1fbbc801f20 static box ptr
               [000040] -----------                                     └──▌  CNS_INT   long   8 Fseq[s_5]

The JIT probably makes a wrong assumption about when the retbuf will be evaluated here.

Author:	jakobbotsch
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[jakobbotsch]

Not super high priority since we do the aggressive spilling today, though there is the potential that with forward sub we can hit bugs due to reliance on the code that I tried to remove in #70932.

[SingleAccretion]

Not sure if the issue above is the same (from the tree alone it looks like it wasn't, and rather caused by the fact insertion of return buffers only works if this cannot interfere with the return buffer tree), but here is another piece of code which relies on this, with the cloning of caller's return buffer trees:

runtime/src/coreclr/jit/importer.cpp

Lines 18178 to 18196 in dbb97ca

	{
	assert(iciCall->gtArgs.HasRetBuffer());
	GenTree* dest = gtCloneExpr(iciCall->gtArgs.GetRetBufferArg()->GetEarlyNode());
	// spill temp only exists if there are multiple return points
	if (fgNeedReturnSpillTemp())
	{
	// if this is the first return we have seen set the retExpr
	if (!impInlineInfo->retExpr)
	{
	impInlineInfo->retExpr =
	impAssignStructPtr(dest, gtNewLclvNode(lvaInlineeReturnSpillTemp, info.compRetType),
	retClsHnd, (unsigned)CHECK_SPILL_ALL);
	}
	}
	else
	{
	impInlineInfo->retExpr = impAssignStructPtr(dest, op2, retClsHnd, (unsigned)CHECK_SPILL_ALL);
	}
	}

[jakobbotsch]

Moving this to future as I don't know of any bugs related to this today.