Fix silent bad codegen VSD possible tailcall converted to normal call (#49552)

The problem was when a VSD interface call returning a multi-byte struct
in registers was initially considered to be a tailcall, but the tailcall
was abandoned in morph due to not enough stack space to store outgoing
arguments, in which case we create a new call return local to store the
return struct, and re-morph the call. In doing so, we forget that we had
already added VSD non-standard args, and re-add them, leaving the originally
added arg as a "normal" arg that shouldn't be there.

So, in summary, for a call A->B, to see this failure, we need:
1. The call is considered a potential tailcall (by the importer)
2. The call requires non-standard arguments that add call argument IR in
   fgInitArgInfo() (e.g., VSD call -- in this case, a generic interface call)
3. We reject the tailcall in fgMorphPotentialTailCall() (e.g., not enough
   incoming arg stack space in A to store B's outgoing args), in this case
   because the first arg is a large struct. We can't reject it earlier,
   due to things like address exposed locals -- we must get far enough
   through the checks to have called fgInitArgInfo() to add the extra
   non-standard arg.
4. B returns a struct in multiple registers (e.g., a 16-byte struct in
   Linux x64 ABI)

The fix is to remove the previously-added non-standard VSD argument IR when
we are preparing to re-morph a call.

There was one other location that was already doing this. I'm a little
worried that there are other places where the non-standard added IR is
not being considered when we clear out the arg info before remorphing.
It seems like there is some risk here. Probably, we should consider a
better way of handling the non-standard arg IR given the need in some
cases to re-morph calls.

I commented out a few cases of:
```
assert(!fgCanFastTailCall(call, nullptr));
```
because this function can call `fgInitArgInfo` which can alter the IR.
That seems dangerous in an assert, which should have any side-effects
like modifying the IR.

Fixes #49078

No SPMI asm diffs.

Author: BruceForstall

src/coreclr/src/jit/gentree.cpp

@@ -1205,6 +1205,38 @@ bool GenTreeCall::Equals(GenTreeCall* c1, GenTreeCall* c2)
     return true;
 }
 
+//--------------------------------------------------------------------------
+// ResetArgInfo: The argument info needs to be reset so it can be recomputed based on some change
+// in conditions, such as changing the return type of a call due to giving up on doing a tailcall.
+// If there is no fgArgInfo computed yet for this call, then there is nothing to reset.
+//
+void GenTreeCall::ResetArgInfo()
+{
+    if (fgArgInfo == nullptr)
+    {
+        return;
+    }
+
+    // We would like to just set `fgArgInfo = nullptr`. But `fgInitArgInfo()` not
+    // only sets up fgArgInfo, it also adds non-standard args to the IR, and we need
+    // to remove that extra IR so it doesn't get added again.
+    //
+    // NOTE: this doesn't handle all possible cases. There might be cases where we
+    // should be removing non-standard arg IR but currently aren't.
+    CLANG_FORMAT_COMMENT_ANCHOR;
+
+#if !defined(TARGET_X86)
+    if (IsVirtualStub())
+    {
+        JITDUMP("Removing VSD non-standard arg [%06u] to prepare for re-morphing call [%06u]\n",
+                Compiler::dspTreeID(gtCallArgs->GetNode()), gtTreeID);
+        gtCallArgs = gtCallArgs->GetNext();
+    }
+#endif // !defined(TARGET_X86)
+
+    fgArgInfo = nullptr;
+}
+
 #if !defined(FEATURE_PUT_STRUCT_ARG_STK)
 unsigned GenTreePutArgStk::getArgSize()
 {

src/coreclr/src/jit/gentree.h

@@ -4502,6 +4502,8 @@ struct GenTreeCall final : public GenTree
         return (gtFlags & GTF_CALL_M_EXP_RUNTIME_LOOKUP) != 0;
     }
 
+    void ResetArgInfo();
+
     unsigned gtCallMoreFlags; // in addition to gtFlags
 
     unsigned char gtCallType : 3;   // value from the gtCallTypes enumeration

src/coreclr/src/jit/morph.cpp

@@ -2390,6 +2390,9 @@ GenTree* Compiler::fgInsertCommaFormTemp(GenTree** ppTree, CORINFO_CLASS_HANDLE
 //    This method only computes the arg table and arg entries for the call (the fgArgInfo),
 //    and makes no modification of the args themselves.
 //
+//    The IR for the call args can change for calls with non-standard arguments: some non-standard
+//    arguments add new call argument IR nodes.
+//
 void Compiler::fgInitArgInfo(GenTreeCall* call)
 {
     GenTreeCall::Use* args;
@@ -6596,6 +6599,9 @@ void Compiler::fgMorphCallInlineHelper(GenTreeCall* call, InlineResult* result)
 //    This function is target specific and each target will make the fastTailCall
 //    decision differently. See the notes below.
 //
+//    This function calls fgInitArgInfo() to initialize the arg info table, which
+//    is used to analyze the argument. This function can alter the call arguments
+//    by adding argument IR nodes for non-standard arguments.
 //
 // Windows Amd64:
 //    A fast tail call can be made whenever the number of callee arguments
@@ -7698,7 +7704,10 @@ GenTree* Compiler::fgMorphTailCallViaHelpers(GenTreeCall* call, CORINFO_TAILCALL
     // We come this route only for tail prefixed calls that cannot be dispatched as
     // fast tail calls
     assert(!call->IsImplicitTailCall());
-    assert(!fgCanFastTailCall(call, nullptr));
+
+    // We want to use the following assert, but it can modify the IR in some cases, so we
+    // can't do that in an assert.
+    // assert(!fgCanFastTailCall(call, nullptr));
 
     bool virtualCall = call->IsVirtual();
 
@@ -7709,11 +7718,7 @@ GenTree* Compiler::fgMorphTailCallViaHelpers(GenTreeCall* call, CORINFO_TAILCALL
     {
         JITDUMP("This is a VSD\n");
 #if FEATURE_FASTTAILCALL
-        // fgInitArgInfo has been called from fgCanFastTailCall and it added the stub address
-        // to the arg list. Remove it now.
-        call->gtCallArgs = call->gtCallArgs->GetNext();
-        // We changed args so recompute info.
-        call->fgArgInfo = nullptr;
+        call->ResetArgInfo();
 #endif
 
         call->gtFlags &= ~GTF_CALL_VIRT_STUB;
@@ -8328,7 +8333,10 @@ void Compiler::fgMorphTailCallViaJitHelper(GenTreeCall* call)
     // We come this route only for tail prefixed calls that cannot be dispatched as
     // fast tail calls
     assert(!call->IsImplicitTailCall());
-    assert(!fgCanFastTailCall(call, nullptr));
+
+    // We want to use the following assert, but it can modify the IR in some cases, so we
+    // can't do that in an assert.
+    // assert(!fgCanFastTailCall(call, nullptr));
 
     // First move the 'this' pointer (if any) onto the regular arg list. We do this because
     // we are going to prepend special arguments onto the argument list (for non-x86 platforms),
@@ -8826,7 +8834,7 @@ GenTree* Compiler::fgMorphCall(GenTreeCall* call)
             //     ret temp
 
             // Force re-evaluating the argInfo as the return argument has changed.
-            call->fgArgInfo = nullptr;
+            call->ResetArgInfo();
 
             // Create a new temp.
             unsigned tmpNum =

src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.cs

@@ -0,0 +1,105 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+
+// Regression test for GitHub issue 49078: https://github.com/dotnet/runtime/issues/49078
+//
+// The problem was when a VSD interface call returning a multi-byte struct in registers was initially considered
+// to be a tailcall, but the tailcall was abandoned in morph due to not enough stack space to store outgoing
+// arguments, in which case we create a new call return local to store the return struct, and re-morph the
+// call. In doing so, we forget that we had already added VSD non-standard args, and re-add them, leaving
+// the originally added arg as a "normal" arg that shouldn't be there.
+//
+// So, in summary, for a call A->B, to see this failure, we need:
+// 
+// 1. The call is considered a potential tailcall (by the importer)
+// 2. The call requires non-standard arguments that add call argument IR in fgInitArgInfo()
+//    (e.g., VSD call -- in this case, a generic interface call)
+// 3. We reject the tailcall in fgMorphPotentialTailCall() (e.g., not enough incoming arg stack space in A
+//    to store B's outgoing args), in this case because the first arg is a large struct. We can't reject
+//    it earlier, due to things like address exposed locals -- we must get far enough through the checks
+//    to have called fgInitArgInfo() to add the extra non-standard arg.
+// 4. B returns a struct in multiple registers (e.g., a 16-byte struct in Linux x64 ABI)
+
+namespace GitHub_49078
+{
+    public struct S16
+    {
+        public IntPtr a;
+        public uint b;
+    }
+
+    public struct BigStruct
+    {
+        public IntPtr a, b, c, d, e, f, g, h, j, k, l, m;
+
+        public BigStruct(IntPtr a1)
+        {
+            a = b = c = d = e = f = g = h = j = k = l = m = a1;
+        }
+    }
+
+    public interface IFoo<T>
+    {
+        public S16 Foo(BigStruct b, int i, int j);
+    }
+
+    public class CFoo<T> : IFoo<T>
+    {
+        public S16 Foo(BigStruct b, int i, int j)
+        {
+            S16 s16;
+            s16.a = (IntPtr)i;
+            s16.b = (uint)j;
+            return s16;
+        }
+    }
+
+    class Test
+    {
+        IFoo<int> m_if = new CFoo<int>();
+        BigStruct m_bs = new BigStruct((IntPtr)1);
+
+        public S16 Caller(int a)
+        {
+            // Add some computation so this is not inlineable (but don't mark it noinline,
+            // which would prevent the tailcall consideration).
+            int i = 7;
+            try
+            {
+                for (int j = 0; j < a; j++)
+                {
+                    i += j;
+                }
+            }
+            finally
+            {
+                i += 2;
+            }
+
+            return m_if.Foo(m_bs, i, a);
+        }
+    }
+
+    class Program
+    {
+        static int Main(string[] args)
+        {
+            Test t = new Test();
+            S16 s = t.Caller(4);
+            long l = (long)s.a + s.b;
+            if (l == 19)
+            {
+                Console.WriteLine("Passed");
+                return 100;
+            }
+            else
+            {
+                Console.WriteLine($"{s.a}, {s.b}, {l}");
+                Console.WriteLine("Failed");
+                return 101;
+            }
+        }
+    }
+}

src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.csproj

@@ -0,0 +1,14 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <CLRTestPriority>1</CLRTestPriority>
+  </PropertyGroup>
+  <PropertyGroup>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>