Don't create cast for tail call return value. #7908

sandreenko · 2016-10-31T19:34:36Z

Don't create cast from small type to bigger stack type when it is tail call tree.
Return value is not presented on the stack and does not require cast.
Fix dotnet/corert#2073.

sandreenko · 2016-10-31T19:36:35Z

@pgavlin @sivarv @dotnet/jit-contrib PTAL.

BruceForstall · 2016-10-31T19:40:30Z

Asm diffs?

sivarv · 2016-10-31T19:58:23Z

src/jit/importer.cpp

-        if (!bIntrinsicImported)
+        // Don't create cast for a variable on the stack if it is intrinsic or tail call.
+        // We assume that the value will not be physically represented there.
+        if (!bIntrinsicImported && !(tailCall && canTailCall))


I am not sure this is the right fix for the following reason:

During importation, impImportCall(), some tail call checks are performed and remaining checks during fgMorphCall(). After the checks in fgMorphCall(), if JIT is committed to honor tail call, it will set a flag (GTF_CALL_M_TAILCALL) on GT_CALL node. Till then there is no guarantee a tail call will be honored.

Say we don't insert the cast during importation even if the tail call is returning a small type. What if the tail call was not honored and it was dispatched as a normal call? Is there anything to ensure that the cast is added back again?

Yes, if we set checkSmallType = true and forbid tail calls in morph, we will get movzx before the commit and mov after this change. It is wrong.
To fix it we can:

Move checkForSmallType logic from importer to morph;

Create the cast during importation and delete it in morph;
What is better?

I am inclined to believe that var=cast(call) is a new tail call pattern that is possible which is not currently handled by morph logic. But what stumps me is that so far we have not encountered such a tail call pattern to expose this issue. Do we have a test case that repro's this issue in non-R2R context? We need it anyways for adding as a unit test case.

I think option #2 is the way to go after we have convinced ourselves that this can happen in non-R2R tail call scenarios by constructing a repro case.

It can't happen if checkForSmallType == false (checkForSmallType = opts.IsJit64Compat() || opts.IsReadyToRun();). So you can see it only in R2R or in JIT64Compat mode. However, in ngen we don't use tail calls and JIT64Compat is outdated.
It is possible to set this flag to true, in this case you will see the assert error in coreclr.
I think it is impossible to reproduce this failure on coreclr without changing source code.

@sandreenko - as discussed offline, please see if we can repro this on desktop side. I am fine even if we can add an IL test case that exposes the issue when R2R compiling the test case binary. As I said earlier, handling this new tail call pattern in morph seems to be the way to go.

sivarv · 2016-11-01T19:04:46Z

@sandreenko - Here is an IL repro that hits the same assert in morph.cpp on desktop

.assembly extern mscorlib { }
.assembly tailcallbug { }

.class private auto ansi beforefieldinit Test.Program
       extends [mscorlib]System.Object
{
  .method private hidebysig static bool  foo(int32 n) cil managed 
  {
    // Code size       24 (0x18)
    .maxstack  8
    IL_0000:  ldarg.0
    IL_0001:  ldc.i4.0
    IL_0002:  bgt.s      IL_000f

    IL_0006:  ldarg.0
    IL_0007:  ldc.i4.1
    IL_0008:  sub
    IL_0009:  call       bool Test.Program::foo(int32)
    IL_000e:  br IL_0017
    IL_000f:  ldarg.0
    IL_0010:  ldc.i4.2
    IL_0011:  sub
    IL_0012:  call       bool Test.Program::foo(int32)
    IL_0017:  ret
  } // end of method Program::foo

  .method private hidebysig static void  Main(string[] args) cil managed
  {
    .entrypoint
    // Code size       9 (0x9)
    .maxstack  8
    IL_0000:  ldc.i4.s   10
    IL_0002:  call       bool Test.Program::foo(int32)
    IL_0007:  pop
    IL_0008:  ret
  } // end of method Program::Main

  .method public hidebysig specialname rtspecialname 
          instance void  .ctor() cil managed
  {
    // Code size       7 (0x7)
    .maxstack  8
    IL_0000:  ldarg.0
    IL_0001:  call       instance void [mscorlib]System.Object::.ctor()
    IL_0006:  ret
  } // end of method Program::.ctor

} // end of class Test.Program

sandreenko · 2016-11-02T23:39:19Z

I added a case for the new pattern "asg -> cast -> call". The pattern with cast already have existed as "return -> cast -> call". We don't need to delete cast because when we do the tailcall, we replace whole statement tree with it.

sandreenko · 2016-11-03T00:06:50Z

@dotnet-bot test Linux ARM Emulator Cross Debug Build

sivarv · 2016-11-03T00:25:39Z

src/jit/morph.cpp

+        noway_assert(
+            (stmtExpr->gtOper == GT_CALL && stmtExpr == call) ||
+            (stmtExpr->gtOper == GT_RETURN &&
+             (stmtExpr->gtOp.gtOp1 == call || stmtExpr->gtOp.gtOp1->gtOp.gtOp1 == call)) ||


Since we are editing this assert, please tighten the assert that stmtExpr->gtOp.gtOp1 is a GT_CAST and stmtExpr->gtOp.gtOp1->gtOp.gtOp1 == call

sivarv · 2016-11-03T00:26:15Z

src/jit/morph.cpp

+            (stmtExpr->gtOper == GT_CALL && stmtExpr == call) ||
+            (stmtExpr->gtOper == GT_RETURN &&
+             (stmtExpr->gtOp.gtOp1 == call || stmtExpr->gtOp.gtOp1->gtOp.gtOp1 == call)) ||
+            (stmtExpr->gtOper == GT_ASG && (stmtExpr->gtOp.gtOp2 == call || stmtExpr->gtOp.gtOp2->gtOp.gtOp1 == call)));


Here too please tighten the assert by adding the check that stmtExpr->gtOp.gtOp2 is a GT_CAST.

In both of the above places we also need to assert that it is not only GT_CAST but a non-overflow checking one. Otherwise, it is not correct to remove GT_CAST.

sivarv · 2016-11-03T00:26:46Z

src/jit/morph.cpp

-                      (stmtExpr->gtOp.gtOp1 == call || stmtExpr->gtOp.gtOp1->gtOp.gtOp1 == call)) ||
-                     (stmtExpr->gtOper == GT_ASG && stmtExpr->gtOp.gtOp2 == call));
+        //    GT_RETURN(GT_CALL(..)) or GT_RETURN(GT_CAST(GT_CALL))
+        //    var = GT_CALL or var = (GT_CAST(GT_CALL)


Nit: closing ")" missing in the comment.

sivarv · 2016-11-03T00:28:42Z

tests/src/JIT/Regression/JitBlue/GitHub_CoreRT_2073/GitHub_CoreRT_2073.il

+} // end of method Test::Main
+
+
+}


Nit: Some special character?

sequense localVariable = cast.int(call.small_type()); return localVaribale is correct tail call pattern.

sandreenko · 2016-11-03T18:57:16Z

@dotnet-bot test Linux ARM Emulator Cross Debug Build

sivarv · 2016-11-03T19:12:54Z

sivarv · 2016-11-03T19:14:12Z

Looks good.

dnfclas added the cla-already-signed label Oct 31, 2016

sivarv reviewed Oct 31, 2016

View reviewed changes

sandreenko force-pushed the fix_tail_call_in_R2R branch from 58b8812 to e4ded66 Compare November 2, 2016 23:19

sivarv reviewed Nov 3, 2016

View reviewed changes

tests/src/JIT/Regression/JitBlue/GitHub_CoreRT_2073/GitHub_CoreRT_2073.il

} // end of method Test::Main

}

Copy link

Member

sivarv Nov 3, 2016

Choose a reason for hiding this comment

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

Nit: Some special character?

sandreenko force-pushed the fix_tail_call_in_R2R branch from e4ded66 to 6807ef6 Compare November 3, 2016 01:34

Correct pattern check

fb33095

sequense localVariable = cast.int(call.small_type()); return localVaribale is correct tail call pattern.

sandreenko force-pushed the fix_tail_call_in_R2R branch from 6807ef6 to fb33095 Compare November 3, 2016 17:36

sandreenko merged commit 552dae9 into dotnet:master Nov 3, 2016

sandreenko deleted the fix_tail_call_in_R2R branch November 3, 2016 20:58

karelz modified the milestone: 2.0.0 Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't create cast for tail call return value. #7908

Don't create cast for tail call return value. #7908

sandreenko commented Oct 31, 2016

sandreenko commented Oct 31, 2016

BruceForstall commented Oct 31, 2016

sivarv Oct 31, 2016

sandreenko Oct 31, 2016

sivarv Oct 31, 2016 •

edited

Loading

sandreenko Oct 31, 2016 •

edited

Loading

sivarv Oct 31, 2016

sivarv commented Nov 1, 2016

sandreenko commented Nov 2, 2016

sandreenko commented Nov 3, 2016

sivarv Nov 3, 2016

sivarv Nov 3, 2016

sivarv Nov 3, 2016

sivarv Nov 3, 2016

sivarv Nov 3, 2016

sandreenko commented Nov 3, 2016

sivarv commented Nov 3, 2016

sivarv commented Nov 3, 2016

Don't create cast for tail call return value. #7908

Don't create cast for tail call return value. #7908

Conversation

sandreenko commented Oct 31, 2016

sandreenko commented Oct 31, 2016

BruceForstall commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sivarv Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

sandreenko Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sivarv commented Nov 1, 2016

sandreenko commented Nov 2, 2016

sandreenko commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko commented Nov 3, 2016

sivarv commented Nov 3, 2016

sivarv commented Nov 3, 2016

sivarv Oct 31, 2016 •

edited

Loading

sandreenko Oct 31, 2016 •

edited

Loading