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.

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

Fix issue with devirtualization and tailcalls #21804

Merged
merged 5 commits into from Jan 11, 2019

Conversation

Projects
None yet
3 participants
@briansull
Copy link
Contributor

briansull commented Jan 4, 2019

When performing devirtualization we can not do both an unboxing optimization and a tail call optimization

  • Explicit tail calls are now checked for and blocked from performing an unboxing operation in impDevirtualizeCall.
  • If late devirtualization calls impDevirtualizeCall with an IMPLICIT_TAILCALL we will clear this flag if we decide to perform the unboxing operation.
@briansull

This comment has been minimized.

Copy link
Contributor

briansull commented Jan 4, 2019

@AndyAyersMS

This comment has been minimized.

Copy link
Member

AndyAyersMS commented Jan 4, 2019

You should make it clearer that this restriction is for explicit tail calls only, where presumably the IL producer is confident that at the point of the explicit tail call there aren't any live references to the caller frame, and if we undo the box, we will introduce one.

And I think this could also happen for late devirt though it would require a different kind of test case.

Seems like you could just check for GTF_CALL_M_EXPLICIT_TAILCALL to inhibit the box undo transform, instead of passing in extra info, and handle it more cleanly and more generally.

@briansull

This comment has been minimized.

Copy link
Contributor

briansull commented Jan 4, 2019

The flag GTF_CALL_M_EXPLICIT_TAILCALL is currently not yet set by the importer when we reach this method.
It currently gets set here: importer.cpp line 8753 and the call to devirtualize is on line 8597.

@briansull

This comment has been minimized.

Copy link
Contributor

briansull commented Jan 4, 2019

The IL for the method with the tailcall is:

****** START compiling Program:Test(int):ref:this (MethodHash=f85425b1)
IL to import:
IL_0000  03                ldarg.1     
IL_0001  8c 04 00 00 01    box          0x1000004
IL_0006  fe 14             tail.       
IL_0008  6f 03 00 00 0a    callvirt     0xA000003
IL_000d  2a                ret         

This IL doesn't contain any live references to the caller frame, however the JIT compiler introduces one via:

Compiler::impImportAndPushBox -- handling BOX(value class) via inline allocate/copy sequence
lvaGrabTemp returning 3 (V03 tmp1) called for Single-def Box Helper.

and then we unbox this, which introduces the live references to the caller frame, 

Source code:

using System;
using System.Runtime.CompilerServices;

class Program
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    public String Test(int val)
    {
        return  /* tailcall */ ((Object) val).ToString();
    }

    static void Main(string[] args)
    {
        Program p = new Program();

        String result = p.Test(42);
        Console.WriteLine("result = " + result);
    }
}
@AndyAyersMS

This comment has been minimized.

Copy link
Member

AndyAyersMS commented Jan 4, 2019

Hmm, ok. We should still fix the late devirt path, and that one needs to work with the call flags. So we need to adapt for one convention or the other.

If it was me I'd probably handle this by passing in a bool isExplicitTailCall and returning a bool result indicating if we unboxed, and then adapt the two call sites to suit.

When performing devirtualization we can not do both an unboxing optim…
…ization and a tail call optimization

Explicit tail calls are now checked for and blocked from performing an unboxing operation in impDevirtualizeCall

If late devirtualization calls impDevirtualizeCall with an IMPLICIT_TAILCALL we will clear this flag if we
decide to perform the unboxing operation.

@briansull briansull force-pushed the briansull:fix-unbox-opt branch from 92bbf5a to 8c12e27 Jan 9, 2019

@briansull

This comment has been minimized.

Copy link
Contributor

briansull commented Jan 10, 2019

@jashook jashook self-requested a review Jan 10, 2019

@@ -8742,7 +8744,6 @@ var_types Compiler::impImportCall(OPCODE opcode,

if (info.compCompHnd->canTailCall(info.compMethodHnd, methHnd, exactCalleeHnd, explicitTailCall))
{
canTailCall = true;

This comment has been minimized.

@briansull

briansull Jan 10, 2019

Contributor

This assignment was redundant (see line 8736 above)

briansull added some commits Jan 10, 2019

@briansull

This comment has been minimized.

Copy link
Contributor

briansull commented Jan 10, 2019

@dotnet-bot Test Ubuntu x64 Checked Innerloop Build and Test

@briansull

This comment has been minimized.

Copy link
Contributor

briansull commented Jan 10, 2019

@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0)

@AndyAyersMS
Copy link
Member

AndyAyersMS left a comment

Looks good overall, just two small nits.

Would be nice to have a test case here too (probably an IL case...)

@@ -20558,6 +20561,12 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,
{
JITDUMP("Now have direct call to boxed entry point, looking for unboxed entry point\n");

if (isTailCall)
{
JITDUMP("Call is an explcit tail call, we cannot perform an unbox\n");

This comment has been minimized.

@AndyAyersMS

AndyAyersMS Jan 10, 2019

Member

Typo: explcit

@@ -20174,6 +20175,7 @@ bool Compiler::IsMathIntrinsic(GenTree* tree)
// contextHandle -- [IN/OUT] context handle for the call. Updated iff call devirtualized.
// exactContextHnd -- [OUT] updated context handle iff call devirtualized
// isLateDevirtualization -- if devirtualization is happening after importation
// isTailCall -- [IN/OUT] true if we plan on using a tail call

This comment has been minimized.

@AndyAyersMS

AndyAyersMS Jan 10, 2019

Member

This is no longer [in/out] -- also rename to isExplictTailCall

@briansull

This comment has been minimized.

Copy link
Contributor

briansull commented Jan 10, 2019

Yes, I do have an IL test case that I will add

@@ -3329,7 +3329,8 @@ class Compiler
unsigned* methodFlags,
CORINFO_CONTEXT_HANDLE* contextHandle,
CORINFO_CONTEXT_HANDLE* exactContextHandle,
bool isLateDevirtualization);
bool isLateDevirtualization,
bool isTailCall);

This comment has been minimized.

@jashook

jashook Jan 10, 2019

Contributor

Is the compiler global isTailCall used?

This comment has been minimized.

@briansull

briansull Jan 11, 2019

Contributor

This parameter is used and it is also renamed as isExplicitTailCall

@briansull briansull referenced this pull request Jan 10, 2019

Closed

New test #21936

briansull added some commits Jan 10, 2019

Code Review feedback
Change test priority to 0
@briansull

This comment has been minimized.

Copy link
Contributor

briansull commented Jan 11, 2019

@dotnet-bot Test OSX10.12 x64 Checked Innerloop Build and Test

@briansull

This comment has been minimized.

Copy link
Contributor

briansull commented Jan 11, 2019

@dotnet-bot Test Windows_NT arm Cross Checked Innerloop Build and Test

@briansull briansull merged commit df88b1f into dotnet:master Jan 11, 2019

30 of 32 checks passed

OSX10.12 x64 Checked Innerloop Build and Test Started.
Details
Windows_NT arm Cross Checked Innerloop Build and Test Started.
Details
CentOS7.1 x64 Checked Innerloop Build and Test Build finished.
Details
CentOS7.1 x64 Debug Innerloop Build Build finished.
Details
Linux-musl x64 Debug Build Build finished.
Details
Tizen armel Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked crossgen_comparison Build and Test Build finished.
Details
Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Release crossgen_comparison Build and Test Build finished.
Details
Ubuntu x64 Checked CoreFX Tests Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Ubuntu16.04 arm64 Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu16.04 arm64 Cross Checked no_tiered_compilation_innerloop Build and Test Build finished.
Details
Windows_NT arm Cross Debug Innerloop Build Build finished.
Details
Windows_NT arm64 Cross Checked Innerloop Build and Test Build finished.
Details
Windows_NT arm64 Cross Debug Innerloop Build Build finished.
Details
Windows_NT x64 Checked CoreFX Tests Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release CoreFX Tests Build finished.
Details
Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x86 Release Innerloop Build and Test Build finished.
Details
Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment