Skip to content

JIT: remove guarded devirtualization restriction for small struct ret… #51157

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.

Sign up for GitHub

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

Merged
AndyAyersMS merged 1 commit into from
Apr 13, 2021
Merged

JIT: remove guarded devirtualization restriction for small struct ret… #51157

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 0 additions & 19 deletions src/coreclr/jit/indirectcalltransformer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,25 +523,6 @@ class IndirectCallTransformer
return;
}

// For now, bail on transforming calls that still appear
// to return structs by value as there is deferred work
// needed to fix up the return type.
//
// See for instance fgUpdateInlineReturnExpressionPlaceHolder.
if (origCall->TypeGet() == TYP_STRUCT)
Copy link
Contributor

@sandreenko sandreenko Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was it protecting only from small struct returns and not from multi-reg?

Copy link
Member Author

@AndyAyersMS AndyAyersMS Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I think it was just the small struct returns?

Copy link
Contributor

@sandreenko sandreenko Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that all struct calls were rejected here because of the one unsupported scenario(small structs). Now we don't need to do anything special for the previously unsupported case but we need to check that it works as expected for all of them.

I have ran the change locally with assert(false) inside the block and we did not go into if (origCall->TypeGet() == TYP_STRUCT) case on framework libraries crossgen/pmi x64/arm64 windows/unix. Is it expected?

However, I saw that we hit it aspnet.run.windows.x64 spmi collection but could not collect the diffs because when we delete this block we start hitting missing answers for the new questions.

Copy link
Member Author

@AndyAyersMS AndyAyersMS Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This optimization requires PGO data, so not too surprising it doesn't fire much for libraries code.

Also by this point any struct return that uses a hidden buffer has been ntransformed and the return type is no longer TYP_STRUCT.

Here's a simple test case where the case does git hit. Run with COMPlus_TieredPGO=1.

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

struct P
{
    public int x;
    public int y;
}

class B
{
    virtual public P get() => new P();
}

class D : B
{
    override public P get() => new P() { x = 67, y = 33 };
}

class X
{
    public static int F(B b) => b.get().x + b.get().y;

    [MethodImpl(MethodImplOptions.NoOptimization)]
    public static int Main()
    {
        B b = new D();
        int r = 0;
        for (int i = 0; i < 100; i++)
        {
            r += F(b);
            Thread.Sleep(15);
        }
        
        r = 0;
        for (int i = 0; i < 10000; i++)
        {
            r += F(b);
        }
        
        r /= 10000;
        
        Console.WriteLine($"r={r}");
        
        return r;
    }
}

I also ran jit-experimental CI below which gives us some coverage, and there were no unexpected failures (save for emptystacktrace\OOMException01 which seems to be failing everywhere).

{
JITDUMP("*** %s Bailing on [%06u] -- can't handle by-value struct returns yet\n", Name(),
compiler->dspTreeID(origCall));
ClearFlag();

// For stub calls restore the stub address
if (origCall->IsVirtualStub())
{
origCall->gtStubCallStubAddr = origCall->gtInlineCandidateInfo->stubAddr;
}
return;
}

likelihood = origCall->gtGuardedDevirtualizationCandidateInfo->likelihood;
assert((likelihood >= 0) && (likelihood <= 100));
JITDUMP("Likelihood of correct guess is %u\n", likelihood);
Expand Down