JIT: GDV reorders null checks of 'this' with arguments

[jakobbotsch]

The following example throws NRE without printing any output when tiered PGO is enabled. It should print "Should be printed" before throwing NRE.

using System;
using System.Runtime.CompilerServices;
using System.Threading;

public class Program
{
    public static void Main()
    {
        for (int i = 0; i < 100; i++)
        {
            long sum = Foo(i => i * 42, true);
            if (i > 30 && i < 40)
                Thread.Sleep(100);
        }

        Foo(null, false);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static long Foo(Func<long, long> f, bool silent)
    {
        long result = 0;
        for (long i = 0; i < 100000; i++)
            result += f(Test(i, silent));

        return result;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static long Test(long i, bool silent)
    {
        if (i == 0 && !silent)
            Console.WriteLine("Should be printed");

        return i;
    }
}

The evaluation order is meant to be:

1. Evaluate this
2. Evaluate arguments
3. Null check this
4. Do call

With GDV, since the guard involves dereferencing 'this', step 3 happens too early. This happens for both vtable, interface and delegate GDV.

Tricky to solve unfortunately. We need to evaluate the guard after the arguments (hard to represent without lots of additional spilling) or add a null check to the guard (expensive). cc @AndyAyersMS @EgorBo

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

Issue Details

---

The following example throws NRE without printing any output when tiered PGO is enabled. It should print "Should be printed" before throwing NRE.

using System;
using System.Runtime.CompilerServices;
using System.Threading;

public class Program
{
    public static void Main()
    {
        for (int i = 0; i < 100; i++)
        {
            long sum = Foo(i => i * 42, true);
            if (i > 30 && i < 40)
                Thread.Sleep(100);
        }

        Foo(null, false);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static long Foo(Func<long, long> f, bool silent)
    {
        long result = 0;
        for (long i = 0; i < 100000; i++)
            result += f(Test(i, silent));

        return result;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static long Test(long i, bool silent)
    {
        if (i == 0 && !silent)
            Console.WriteLine("Should be printed");

        return i;
    }
}

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

[jakobbotsch]

This actually makes early expansion of delegates quite hard since it requires us to evaluate all arguments with side effects before computing the new this pointer. We're not really set up to represent this well.

[jakobbotsch]

Example with interface GDV:

using System;
using System.Runtime.CompilerServices;
using System.Threading;

public class Program
{
    public static void Main()
    {
        for (int i = 0; i < 100; i++)
        {
            long sum = Foo(new C(), true);
            if (i > 30 && i < 40)
                Thread.Sleep(100);
        }

        Foo(null, false);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static long Foo(IFace iface, bool silent)
    {
        long result = 0;
        for (long i = 0; i < 100000; i++)
            result += iface.Test(SideEffect(i, silent));

        return result;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static long SideEffect(long i, bool silent)
    {
        if (i == 0 && !silent)
            Console.WriteLine("Should be printed");

        return i;
    }
}

public interface IFace
{
    long Test(long arg);
}

public class C : IFace
{
    public long Test(long arg)
    {
        return arg;
    }
}

[jakobbotsch]

Early spilling might not be a problem. We already spill most arguments quite aggressively so spilling another for GDV candidates is probably not too bad.

[JulieLeeMSFT]

@jakobbotsch, do we need to fix this in .NET 7?

[jakobbotsch]

@jakobbotsch, do we need to fix this in .NET 7?

We've had this behavior for quite some time without seeing any user reports, but I'd say it's a quite significant deviation in semantics. I'll discuss it with @AndyAyersMS in a bit.

[jakobbotsch]

We spoke offline and agreed to put this in 8.0 milestone since we haven't had any user reports in all of .NET 6 about this behavior mismatch.