dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.15k stars 4.71k forks source link

CoreRT shows a problem with inlined tail calls. #8258

Closed sandreenko closed 4 years ago

sandreenko commented 7 years ago

Run ILC in Release mode with Debug RyuJit with such test:

using System;
using System.Reflection;
using System.Runtime.CompilerServices;

internal class ConstructorAttribute : Attribute
{
}

internal class Program
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    private bool GetAttributeConstructor(ConstructorInfo c)
    {
        return CustomAttributeExtensions.IsDefined(c, typeof(ConstructorAttribute), true);
    }

    private static void Main(string[] args)
    {
        new Program().GetAttributeConstructor(typeof(Program).GetTypeInfo().GetConstructor(Array.Empty<Type>()));
    }
}

Set ngenDump for GetAttributeConstructor, you will see:

Invoking compiler for the inlinee method Internal.LowLevelLinq.LowLevelEnumerable:Any(ref):bool :
IL to import:
IL_0000  02                ldarg.0
IL_0001  6f 83 00 00 0a    callvirt     0xA000083
IL_0006  0a                stloc.0
IL_0007  06                ldloc.0
IL_0008  6f d3 1d 00 06    callvirt     0x6001DD3
IL_000d  2a                ret

and


***** BB01, stmt 7
               [000076] ------------             *  stmtExpr  void  (IL   ???...  ???)
               [000073] --C---------             |  /--*  cast      int <- bool <- int
               [000067] --C-G-------             |  |  \--*  call      int    Internal.LowLevelLinq.LowLevelEnumerable.Any
               [000068] ------------ arg0        |  |     +--*  const(h)  long   0x427DA0 method
               [000066] ------------ arg1        |  |     \--*  lclVar    ref    V07 tmp5
               [000075] -AC---------             \--*  =         bool
               [000074] D------N----                \--*  lclVar    bool   V05 tmp3

***** BB01, stmt 8
               [000025] ------------             *  stmtExpr  void  (IL   ???...  ???)
               [000024] --C---------             \--*  return    int
               [000023] --C---------                \--*  cast      int <- bool <- int
               [000077] ------------                   \--*  lclVar    int    V05 tmp3

Call 67 will be marked as tail call, but the morph condition for the return statement doesn't expect a cast and will blow up.

It works fine with the release mode of the jit. I am not yet sure about the right fix, but there is strange code in fgMorphCall:

            // Peel off casts
            while (treeWithCall->gtOper == GT_CAST)
            {
                noway_assert(!treeWithCall->gtOverflow());
                treeWithCall = treeWithCall->gtGetOp1();
            }

It is inside the #ifdef Debug branch, that initially contained only assert checks, but some real code was added recently (#9405). @AndyAyersMS Is it right that we peel casts only in debug and only for the call node, but not for the return statement?

cc @MichalStrehovsky @dotnet/jit-contrib.

AndyAyersMS commented 7 years ago

The call side peeling is ok -- it only feeds an assert that the call is found in the right kind of tree.

If the return gets split from the call as you show above then the logic on the return side -- assuming you're hitting the final noway_assert below:

        // Delete GT_RETURN  if any
        if (nextMorphStmt != nullptr)
        {
            GenTreePtr retExpr = nextMorphStmt->gtStmtExpr;
            noway_assert(retExpr->gtOper == GT_RETURN);

            // If var=call, then the next stmt must be a GT_RETURN(TYP_VOID) or GT_RETURN(var).
            // This can occur if impSpillStackEnsure() has introduced an assignment to a temp.
            if (stmtExpr->gtOper == GT_ASG && info.compRetType != TYP_VOID)
            {
                noway_assert(stmtExpr->gtGetOp1()->OperIsLocal());
                noway_assert(stmtExpr->gtGetOp1()->AsLclVarCommon()->gtLclNum ==
                             retExpr->gtGetOp1()->AsLclVarCommon()->gtLclNum);
            }

-- will need a similar update: you will need to tunnel through casts on the retExpr op1 side to find a non-cast and then assert it is the expected local var.