JIT: Give up on the tail call if there are unexpected blocks after it

[EgorBo]

Fixes #50492 assert.

It only reproduces with the following env. vars:

COMPlus_TieredCompilation=1
COMPlus_TC_QuickJitForLoops=1
COMPlus_ReadyToRun=0
COMPlus_ZapDisable=1
COMPlus_TieredPGO=1

on ILSpy app (https://github.com/icsharpcode/AvaloniaILSpy) after some time randomly clicking, doesn't repro with TieredPGO=0

JitDump of a failed method https://gist.github.com/EgorBo/f0f58d04ec537ae815203f947309df65
(initially I wanted to perform fgCompactBlocks in such cases, but the sequence of blocks after the taill call is more complicated than fgCompact can handle)

I don't know how to make a unit test out of it, perhaps I can write it in IL.

diffs are empty.

/cc @AndyAyersMS @dotnet/jit-contrib

[AndyAyersMS]

Can you share out a dump? I am curious how this happens (perhaps GDV of a tail call candidate?)

[EgorBo]

Can you share out a dump? I am curious how this happens (perhaps GDV of a tail call candidate?)

Ah, yes, it happens because of multiple GDVs, trying to make a quick repro - I guess we just need to reset the tailcall prefix in such cases?

[AndyAyersMS]

Surprised we haven't run across this already somewhere. Seems like it ought to be easy to repro.

I guess we just need to reset the tailcall prefix in such cases?

We could also try and fix the GDV transform to not add blocks after the call, if the call is a tail call candidate.

[EgorBo]

Seems like it ought to be easy to repro.

I couldn't manage to create a repro for it 🙁 maybe I'm missing something.

We could also try and fix the GDV transform to not add blocks after the call, if the call is a tail call candidate.

Do you mean to not inline tail calls after GDV?

Minimal repro: create a net5.0 console app, add ICSharpCode.Decompiler nuget package.

using System;
using System.Threading;
using ICSharpCode.Decompiler;
using ICSharpCode.Decompiler.CSharp;
using ICSharpCode.Decompiler.TypeSystem;

class Program
{
    static void Main()
    {
        var decompiler = new CSharpDecompiler(typeof(object).Assembly.Location, new DecompilerSettings());
        for (int i = 0; i < 100; i++)
        {
            decompiler.DecompileTypeAsString(new FullTypeName("System.__ComObject"));
            Thread.Sleep(16);
        }
        Console.WriteLine("done");
    }
}

To get a jit dump:

DOTNET_JitDump=ICSharpCode.Decompiler.TypeSystem.Implementation.NullabilityAnnotatedType:AcceptVisitor(ICSharpCode.Decompiler.TypeSystem.TypeVisitor):ICSharpCode.Decompiler.TypeSystem.IType:this

[AndyAyersMS]

Here's a relatively simple repro

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

class B 
{
    public virtual int F(B[] b, bool p)
    {
        int r = 0;
        for (int i = 0; i < 10; i++)
        {
            r = i * i;
        }
        for (int i = 0; i < 10; i++)
        {
            r = i * i;
        }
        return r;
    }
}

class D : B
{
    public override int F(B[] b, bool p) => p ? b[2].F(b, p) : 5;
}

class E : D
{
    public override int F(B[] b, bool p) => b[1].F(b, p);
}

class X
{
    static B[] b = { new E(), new D(), new B() };

    public static int Main()
    {
        int r = 0;

        for (int i = 0; i < 100; i++)
        {
            r += b[0].F(b, true);
            Thread.Sleep(15);
        }

        for (int i = 0; i < 100; i++)
        {
            r += b[0].F(b, true);
            Thread.Sleep(15);
        }

        Console.WriteLine($"r = {r}");
        return r;
    }
}

The problem arises in E.F. When we have implicit tail call candidates that are also GDV candidates, and we do multiple levels of GDV expansion, we can end up with multiple blocks between the call and the return, each of which just copies the return value from one temp to another.

In the example above, E.F expands its call via GDV to check for D, then on the happy path, inlines D.F as a tail call candidate; then expands the the call to F in D.F via GDV, leaving a call to B.F in tail position on the happy path. This call doesn't get inlined, and we hit the assert as it is several blocks away from the return by this point.

Since all this happens on the happy path, I think we should try and fix this instead of bailing out on the tail call, either by coalescing temps during GDV (if the return value of the original call is already going into a return value temp, just use that for the GDV temp, instead of creating a new one), or by making the check in morph recognize that we can see these sorts of chains of returns.

[EgorBo]

Since all this happens on the happy path, I think we should try and fix this instead of bailing out on the tail call, either by coalescing temps during GDV (if the return value of the original call is already going into a return value temp, just use that for the GDV temp, instead of creating a new one), or by making the check in morph recognize that we can see these sorts of chains of returns.

Thanks for the repro! I've implemented the second option but will check if I can easily track the fact that GT_RET_EXPR is already assigned to a temp so we don't need to introduce another one.

[AndyAyersMS]

I have a fix for the first option in the works. It leverages the preexistingSpillTemp mechanism on the inline candidate.

[AndyAyersMS]

Here's what I came up with: main...AndyAyersMS:FixGDVTailCallIssue.

It might make sense to take both our changes...

[AndyAyersMS]

Consider picking up my fix as well.

[EgorBo]

Consider picking up my fix as well.

Nice! Thanks, I've cherry-picked it to my branch.

[AndyAyersMS]

Changes LGTM, but since some of this is my code, maybe we should get a second review.

[AndyAyersMS]

@dotnet/jit-contrib anyone else want to review? This has changes from both me and @EgorBo.

[AndyAyersMS]

FYI we also hit this issue in weekend PGO testing...

https://dev.azure.com/dnceng/public/_build/results?buildId=1072201&view=ms.vss-test-web.build-test-results-tab