Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

JIT: add some devirtualization info to the inline context #20395

Merged
merged 2 commits into from
Oct 13, 2018

Conversation

AndyAyersMS
Copy link
Member

Allows the jit to remember which calls were devirtualized and which
of those were then optimized to use an unboxed entry point. This info
is then dumped out as part of the inline tree.

Also remove some of the clutter from the COMPlus_JitPrintInlinedMethods
output stream -- we don't need to see both the in-stream results and
the final results, and we don't really need to know about the budget.
This information is still dumped for COMPlus_JitDump.

Allows the jit to remember which calls were devirtualized and which
of those were then optimized to use an unboxed entry point. This info
is then dumped out as part of the inline tree.

Also remove some of the clutter from the COMPlus_JitPrintInlinedMethods
output stream -- we don't need to see both the in-stream results and
the final results, and we don't really need to know about the budget.
This information is still dumped for COMPlus_JitDump.
@AndyAyersMS
Copy link
Member Author

@sandreenko PTAL
cc @dotnet/jit-contrib @adamsitnik

For this example

using System;

interface IPrint 
{
    void Print();
}

struct X<T> : IPrint
{
    public X(T t) { _t = t; }
    public void Print() { Console.WriteLine(_t); }
    T _t;
}

class Y
{
    static int Main()
    {
        var s = new X<string>("hello, world!");
        ((IPrint)s).Print();
        return 100;
    }
}

the inline tree now shows the call to Print was devirtualized and then updated to invoke the unboxed entry:

Inlines into 06000004 Y:Main():int
  [1 IL=0005 TR=000008 06000002] [below ALWAYS_INLINE size] X`1[__Canon][System.__Canon]:.ctor(ref):this
  [2 IL=0015 TR=000027 06000003] [profitable inline devirt unboxed] X`1[__Canon][System.__Canon]:Print():this
    [0 IL=0011 TR=000049 06000080] [FAILED: noinline per IL/cached result] System.Console:WriteLine(ref)

We can think about propagating these bits back to the runtime so they can eventually be surfaced in the ETL, but more work is needed (likely you'd also want the failure reasons).

@drieseng
Copy link

@AndyAyersMS Unrelated, but where can I find docs on the failed inline reasons (such as "noinline per IL/cached result")?

@AndyAyersMS
Copy link
Member Author

@drieseng There aren't any docs per se -- the set of possible failure reasons is here (ignore the "information" cases). Many of the failure reasons are uncommon and obscure.

"No inline per IL / cached result" indicates EITHER that the method was marked with NoInlining OR that at some point in the past the jit attempted to inline the method and discovered that the method could not be inlined OR that the jit decided the method would never be a good inline.

Note "earlier" can mean the jit attempted to inline the method during crossgen OR realized the method could never be inlined when crossgenning the method. So if a method is not marked as noinline and isn't getting inlined it can sometimes be a challenge to figure out why it is not getting inlined.

The noinline caching is done to help speed up jitting, so that the jit doesn't spend time trying to inline methods that ultimately can't or won't be inlined. And IIRC it has a significant positive impact on jit time.

I have ambitions (#6095) of splitting up this particular failure category so that instead of "cached" noinline results we can surface the actual failure reason, but haven't gotten around to it.

@AndyAyersMS
Copy link
Member Author

FWIW it might be an interesting project to write test cases for every possible inline failure so we have something we can refer to.... note some of them will have to be written in IL.

Copy link

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

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

LGTM with few comments.

src/jit/flowgraph.cpp Show resolved Hide resolved
failedContext->m_Sibling = parentContext->m_Child;
parentContext->m_Child = failedContext;
failedContext->m_Child = nullptr;
failedContext->m_Offset = stmt->gtStmtILoffsx;

Choose a reason for hiding this comment

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

While you are here, could you please fix headers/signature for
InlineContext* InlineStrategy::NewSuccess(InlineInfo* inlineInfo)
and
InlineContext* InlineStrategy::NewFailure(GenTreeStmt* stmt, InlineResult* inlineResult),
they probably should be the same (and the headers are), but NewSuccess doesn't have stmt - statement containing call being inlined as argument.

Then maybe it will be obvious how to extract this two similar initialization lists into one method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll update the comment, but are you suggesting common out some of the work these two methods do?

It may be clunky as they don't have access to the same data.

Choose a reason for hiding this comment

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

I think you can delete GenTreeStmt* stmt arg from NewFailure and do what NewSuccess does:
GenTreeStmt* stmt = inlineInfo->iciStmt;
calleeContext->m_Offset = stmt->AsStmt()->gtStmtILoffsx; <- looks like we do not need this cast here, it is already a statement.

Copy link
Member Author

Choose a reason for hiding this comment

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

We currently don't have access to the InlineInfo for the failure cases.

I'll remove the cast too.

src/jit/inline.h Outdated Show resolved Hide resolved
@AndyAyersMS
Copy link
Member Author

@sandreenko Made a few updates based on your feedback.

Copy link

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

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

Thank you.

@AndyAyersMS
Copy link
Member Author

OSX failure is:

00:15:17.573   System.IO.FileLoadException: Could not load file or assembly 'Interop.ArrayMarshalling.XUnitWrapper, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'. An operation is not legal in the current state. (Exception from HRESULT: 0x80131509 (COR_E_INVALIDOPERATION))

ubuntu arm failure is similar....

00:50:40.183   System.IO.FileLoadException: Could not load file or assembly 'baseservices.threading.XUnitWrapper, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'. An operation is not legal in the current state. (Exception from HRESULT: 0x80131509 (COR_E_INVALIDOPERATION))

windows arm failure looks like an infrastructure problem:

00:00:12.429  > git init C:\j\workspace\arm_cross_che---c874ca48 # timeout=10
00:00:12.662 
ERROR: Error cloning remote repo 'origin'
00:00:12.662 hudson.plugins.git.GitException: Could not init C:\j\workspace\arm_cross_che---c874ca48
00:00:12.662 	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$5.execute(CliGitAPIImpl.java:717)
00:00:12.662 	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$2.execute(CliGitAPIImpl.java:511)

None of these seem related. First two have been reported in #20392.

@AndyAyersMS AndyAyersMS merged commit 10a8e25 into dotnet:master Oct 13, 2018
@AndyAyersMS AndyAyersMS deleted the FlagDevirtualizedCalls branch October 13, 2018 01:30
A-And pushed a commit to A-And/coreclr that referenced this pull request Nov 20, 2018
Allows the jit to remember which calls were devirtualized and which
of those were then optimized to use an unboxed entry point. This info
is then dumped out as part of the inline tree.

Also remove some of the clutter from the COMPlus_JitPrintInlinedMethods
output stream -- we don't need to see both the in-stream results and
the final results, and we don't really need to know about the budget.
This information is still dumped for COMPlus_JitDump.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants