Tail call test failure fixes.

6 files changed, 107 insertions, 87 deletions

diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp
index acea3cbfdf..217ac21f27 100644
--- a/src/jit/morph.cpp
+++ b/src/jit/morph.cpp
@@ -7105,81 +7105,102 @@ GenTreePtr          Compiler::fgMorphCall(GenTreeCall* call)
         }
 #endif
 
-        GenTreePtr stmtExpr = fgMorphStmt->gtStmt.gtStmtExpr;
-        bool deleteReturn = false;
-        if (info.compRetBuffArg != BAD_VAR_NUM)
-        {
-            // In this case we simply have a call followed by a return.
-            noway_assert(fgMorphStmt->gtNext->gtStmt.gtStmtExpr->gtOper == GT_RETURN);
-            deleteReturn = true;
-        }
-        else if ((stmtExpr->gtOper == GT_ASG) && (fgMorphStmt->gtNext != nullptr))
-        {
-            GenTreePtr nextStmtExpr = fgMorphStmt->gtNext->gtStmt.gtStmtExpr;
-            noway_assert(nextStmtExpr->gtOper == GT_RETURN);
-            // In this case we have an assignment of the result of the call, and then a return of the  result of the assignment.
-            // This can occur if impSpillStackEnsure() has introduced an assignment to a temp.
-            noway_assert(stmtExpr->gtGetOp1()->OperIsLocal() &&
-                         nextStmtExpr->OperGet() == GT_RETURN &&
-                         nextStmtExpr->gtGetOp1() != nullptr &&
-                         nextStmtExpr->gtGetOp1()->OperIsLocal() &&
-                         stmtExpr->gtGetOp1()->AsLclVarCommon()->gtLclNum == nextStmtExpr->gtGetOp1()->AsLclVarCommon()->gtLclNum);
-            deleteReturn = true;
-        }
-        if (deleteReturn)
-        {
-            fgRemoveStmt(compCurBB, fgMorphStmt->gtNext);
-        }
+        
+        GenTreePtr stmtExpr = fgMorphStmt->gtStmt.gtStmtExpr; 
+        
+#ifdef DEBUG
+        // Tail call needs to be in one of the following IR forms
+        //    Either a call stmt or
+        //    GT_RETURN(GT_CALL(..)) or 
+        //    var = call
+        noway_assert((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));  
+#endif
 
         // For void calls, we would have created a GT_CALL in the stmt list.
         // For non-void calls, we would have created a GT_RETURN(GT_CAST(GT_CALL)).
         // For calls returning structs, we would have a void call, followed by a void return.
         // For debuggable code, it would be an assignment of the call to a temp
         // We want to get rid of any of this extra trees, and just leave
-        // the call
-        
-        bool tailCallFollowedByPopAndRet = false;
-        GenTreePtr stmt;
+        // the call.
+        GenTreePtr nextMorphStmt = fgMorphStmt->gtNext;
 
-#ifdef DEBUG
-        noway_assert((stmtExpr->gtOper == GT_CALL && stmtExpr == call) ||   // Either a call stmt
-                     (stmtExpr->gtOper == GT_RETURN && (stmtExpr->gtOp.gtOp1 == call || stmtExpr->gtOp.gtOp1->gtOp.gtOp1 == call)) || // GT_RETURN(GT_CALL(..))
-                     (stmtExpr->gtOper == GT_ASG && stmtExpr->gtOp.gtOp2 == call));  // or var = call
-#endif
-        
 #ifdef _TARGET_AMD64_
-        if ((stmtExpr->gtOper == GT_CALL) && (fgMorphStmt->gtNext != nullptr))
-        {
-            // We have a stmt node after a tail call node. This must be a tail call occuring
-            // in the following IL pattern
-            //    tail.call
-            //    pop
-            //    ret
-            // Since tail prefix is honored, we can get rid of the remaining two stmts
-            // corresponding to pop and ret. Note that 'pop' may or may not result in 
-            // a new statement (see impImportBlockCode() for details).
-            stmt = fgMorphStmt->gtNext;
-            if (stmt->gtNext != nullptr) 
-            {
-                // We have a pop tree. 
-                // It must be side effect free.
-                GenTreePtr ret = stmt->gtNext;
-                noway_assert((stmt->gtStmt.gtStmtExpr->gtFlags & GTF_ALL_EFFECT) == 0);
-                fgRemoveStmt(compCurBB, stmt);
-                stmt = ret;
-            }
-            noway_assert(stmt->gtStmt.gtStmtExpr->gtOper == GT_RETURN);
-            fgRemoveStmt(compCurBB, stmt);
-            
-            tailCallFollowedByPopAndRet = true;
+        // Legacy Jit64 Compat:
+        // There could be any number of GT_NOPs between tail call and GT_RETURN.
+        // That is tail call pattern could be one of the following:
+        //  1) tail.call, nop*, ret 
+        //  2) tail.call, nop*, pop, nop*, ret  
+        //  3) var=tail.call, nop*, ret(var) 
+        //  4) var=tail.call, nop*, pop, ret
+        //
+        // See impIsTailCallILPattern() for details on tail call IL patterns
+        // that are supported.
+        if ((stmtExpr->gtOper == GT_CALL) || (stmtExpr->gtOper == GT_ASG))
+        {
+            // First delete all GT_NOPs after the call
+            GenTreePtr morphStmtToRemove = nullptr;
+            while (nextMorphStmt != nullptr)
+            {
+                GenTreePtr nextStmtExpr = nextMorphStmt->gtStmt.gtStmtExpr;
+                if (!nextStmtExpr->IsNothingNode())
+                {
+                    break;
+                }
+
+                morphStmtToRemove = nextMorphStmt;
+                nextMorphStmt = morphStmtToRemove->gtNext;
+                fgRemoveStmt(compCurBB, morphStmtToRemove);
+            }
+
+            // Check to see if there is a pop.
+            // Since tail call is honored, we can get rid of the stmt corresponding to pop.
+            if (nextMorphStmt != nullptr && nextMorphStmt->gtStmt.gtStmtExpr->gtOper != GT_RETURN)
+            {                
+                // Note that pop opcode may or may not result in a new stmt (for details see
+                // impImportBlockCode()). Hence, it is not possible to assert about the IR
+                // form generated by pop but pop tree must be side-effect free so that we can
+                // delete it safely.
+                GenTreePtr popStmt = nextMorphStmt;
+                nextMorphStmt = nextMorphStmt->gtNext;
+
+                noway_assert((popStmt->gtStmt.gtStmtExpr->gtFlags & GTF_ALL_EFFECT) == 0);
+                fgRemoveStmt(compCurBB, popStmt);
+            }
+
+            // Next delete any GT_NOP nodes after pop
+            while (nextMorphStmt != nullptr)
+            {
+                GenTreePtr nextStmtExpr = nextMorphStmt->gtStmt.gtStmtExpr;
+                if (!nextStmtExpr->IsNothingNode())
+                {
+                    break;
+                }
+
+                morphStmtToRemove = nextMorphStmt;
+                nextMorphStmt = morphStmtToRemove->gtNext;
+                fgRemoveStmt(compCurBB, morphStmtToRemove);
+            }
         }
-#else //!TARGET_AMD64_
+#endif // _TARGET_AMD64_
 
-#ifdef DEBUG
-        noway_assert(fgMorphStmt->gtNext == nullptr);
-#endif
+        // Delete GT_RETURN  if any
+        if (nextMorphStmt != nullptr)
+        {
+            GenTreePtr retExpr = nextMorphStmt->gtStmt.gtStmtExpr;
+            noway_assert(retExpr->gtOper == GT_RETURN);
 
-#endif //!_TARGET_AMD64_
+            // 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);
+            }
+
+            fgRemoveStmt(compCurBB, nextMorphStmt);
+        }
 
         fgMorphStmt->gtStmt.gtStmtExpr = call;
 
@@ -7229,16 +7250,10 @@ GenTreePtr          Compiler::fgMorphCall(GenTreeCall* call)
         }
 
         // For non-void calls, we return a place holder which will be
-        // used by the parent GT_RETURN node of this call.  This should
-        // not be done for tail calls occuring in the following IL pattern,
-        // since this pattern is supported only in void returning methods.
-        //    tail.call
-        //    pop
-        //    ret
+        // used by the parent GT_RETURN node of this call. 
 
         GenTree* result = call;
-
-        if (!tailCallFollowedByPopAndRet && (callType != TYP_VOID) && info.compRetType != TYP_VOID)
+        if (callType != TYP_VOID && info.compRetType != TYP_VOID)
         {
 #ifdef _TARGET_ARM_
             // Return a dummy node, as the return is already removed.
diff --git a/tests/issues.targets b/tests/issues.targets
index 04463e8892..74c7c4588a 100644
--- a/tests/issues.targets
+++ b/tests/issues.targets
@@ -1,9 +1,6 @@
 <Project DefaultTargets = "GetListOfTestCmds"
     xmlns="http://schemas.microsoft.com/developer/msbuild/2003" >
     <ItemGroup Condition="'$(XunitTestBinBase)' != ''">
-        <ExcludeList Include="$(XunitTestBinBase)\JIT\opt\Tailcall\TailcallVerifyWithPrefix\TailcallVerifyWithPrefix.cmd" >
-             <Issue>2329</Issue>
-        </ExcludeList>
         <ExcludeList Include="$(XunitTestBinBase)\JIT\Methodical\delegate\_simpleoddpower_il_d\_simpleoddpower_il_d.cmd" >
              <Issue>2407</Issue>
         </ExcludeList>
@@ -290,5 +287,8 @@
       <ExcludeList Include="$(XunitTestBinBase)\JIT\SIMD\CircleInConvex_ro\CircleInConvex_ro.cmd">
         <Issue>4992</Issue>
       </ExcludeList>
+      <ExcludeList Include="$(XunitTestBinBase)\JIT\opt\Tailcall\TailcallVerifyWithPrefix\TailcallVerifyWithPrefix.cmd" >
+         <Issue>x86 JIT doesn't support tail call opt</Issue>
+      </ExcludeList>
     </ItemGroup>
 </Project>
diff --git a/tests/src/JIT/opt/Tailcall/TailcallVerifyWithPrefix.il b/tests/src/JIT/opt/Tailcall/TailcallVerifyWithPrefix.il
index 487a009648..bf629916cd 100644
--- a/tests/src/JIT/opt/Tailcall/TailcallVerifyWithPrefix.il
+++ b/tests/src/JIT/opt/Tailcall/TailcallVerifyWithPrefix.il
@@ -96,7 +96,7 @@
       IL_0031:  ldstr      "Caller"
       IL_0036:  callvirt   instance int32 [mscorlib]System.String::IndexOf(string)
       IL_003b:  ldc.i4.m1
-      IL_003c:  bne.un.s   IL_006e
+      IL_003c:  beq.s   IL_006e
 
       IL_003e:  ldstr      "FAILED: Found the word 'Caller' in the stacktrace."
       IL_0043:  call       void [System.Console]System.Console::WriteLine(string)
@@ -173,7 +173,7 @@
   {
     // Code size       154 (0x9a)
     .maxstack  3
-    .locals init ([0] class [mscorlib]System.Exception e)
+    .locals init ([0] class [System.Console]System.Exception e)
     IL_0000:  ldstr      "Executing Condition18.Test2 - Caller(imperative se"
     + "curity): Arguments: None - ReturnType: 3 byte struct; Callee: Arguments"
     + ": None - ReturnType: 3 byte struct"
@@ -6686,8 +6686,10 @@
     IL_011d:  box        [mscorlib]System.Int16
     IL_0122:  stelem.ref
     IL_0123:  ldloc.0
-    IL_0124:  call       void [System.Console]System.Console::WriteLine(string,
-                                                                  object[])
+    IL_0124:  call       void [System.Console]System.Console::Write(string)
+              callvirt   instance string [mscorlib]System.Object::ToString()
+              call       void [System.Console]System.Console::WriteLine(string)
+
     IL_0129:  ldc.i4.1
     IL_012a:  volatile.
     IL_012c:  ldsfld     int32 modreq([mscorlib]System.Runtime.CompilerServices.IsVolatile) TailcallVerify.Condition7::zero
@@ -8793,17 +8795,19 @@
       IL_0035:  ldstr      "FAILED: The fields in the return type struct have "
       + "the wrong values."
       IL_003a:  call       void [System.Console]System.Console::WriteLine(string)
-      IL_003f:  ldstr      "v.i1: {0} != byte.MinValue || v.i2: {1} != short.M"
+      IL_003f:  ldstr      "v.i1: != byte.MinValue || v.i2: != short.M"
       + "axValue"
+                call       void [System.Console]System.Console::WriteLine(string)
       IL_0044:  ldloca.s   v
       IL_0046:  ldfld      uint8 TailcallVerify.ValueType3Bytes::i1
       IL_004b:  box        [mscorlib]System.Byte
+                callvirt   instance string [mscorlib]System.Object::ToString()
+                call       void [System.Console]System.Console::WriteLine(string)
       IL_0050:  ldloca.s   v
       IL_0052:  ldfld      int16 TailcallVerify.ValueType3Bytes::i2
       IL_0057:  box        [mscorlib]System.Int16
-      IL_005c:  call       void [System.Console]System.Console::WriteLine(string,
-                                                                    object,
-                                                                    object)
+                callvirt   instance string [mscorlib]System.Object::ToString()
+      IL_005c:  call       void [System.Console]System.Console::WriteLine(string)
       IL_0061:  leave.s    IL_0082
 
     }  // end .try
diff --git a/tests/src/JIT/opt/Tailcall/TailcallVerifyWithPrefix.ilproj b/tests/src/JIT/opt/Tailcall/TailcallVerifyWithPrefix.ilproj
index 6d8d6b608c..deb7d8d742 100644
--- a/tests/src/JIT/opt/Tailcall/TailcallVerifyWithPrefix.ilproj
+++ b/tests/src/JIT/opt/Tailcall/TailcallVerifyWithPrefix.ilproj
@@ -28,8 +28,9 @@
     </CodeAnalysisDependentAssemblyPaths>
   </ItemGroup>
   <PropertyGroup>
-    
-    
+     <DebugType>None</DebugType>
+     <Optimize>True</Optimize>
+     <JitOptimizationSensitive>true</JitOptimizationSensitive>    
   </PropertyGroup>
   <ItemGroup>
     <Compile Include="TailcallVerifyWithPrefix.il TailcallVerifyTransparentLibraryWithPrefix.il TailcallVerifyVerifiableLibraryWithPrefix.il" />
diff --git a/tests/x86_jit32_issues.targets b/tests/x86_jit32_issues.targets
index 06074ddf82..abb8452024 100644
--- a/tests/x86_jit32_issues.targets
+++ b/tests/x86_jit32_issues.targets
@@ -149,7 +149,7 @@
              <Issue>needs triage</Issue>
         </ExcludeList>
         <ExcludeList Include="$(XunitTestBinBase)\JIT\opt\Tailcall\TailcallVerifyWithPrefix\TailcallVerifyWithPrefix.cmd" >
-             <Issue>needs triage</Issue>
+             <Issue>x86 JIT doesn't support tail call opt</Issue>
         </ExcludeList>
         <ExcludeList Include="$(XunitTestBinBase)\JIT\Methodical\cctor\misc\global_il_d\global_il_d.cmd" >
              <Issue>needs triage</Issue>
diff --git a/tests/x86_legacy_backend_issues.targets b/tests/x86_legacy_backend_issues.targets
index 1eb3afc60c..23805fb4c3 100644
--- a/tests/x86_legacy_backend_issues.targets
+++ b/tests/x86_legacy_backend_issues.targets
@@ -242,7 +242,7 @@
       <Issue>needs triage</Issue>
     </ExcludeList>
     <ExcludeList Include="$(XunitTestBinBase)\JIT\opt\Tailcall\TailcallVerifyWithPrefix\TailcallVerifyWithPrefix.cmd">
-      <Issue>needs triage</Issue>
+      <Issue>x86 JIT doesn't support tail call opt</Issue>
     </ExcludeList>
     <ExcludeList Include="$(XunitTestBinBase)\Threading\ThreadStatics\ThreadStatic06\ThreadStatic06.cmd Timed Out">
       <Issue>needs triage</Issue>