diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 1561d69a65d99..ec9640bc1172c 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/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() { diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h index c7d5f2cf3a3a0..38836d63dc0ef 100644 --- a/src/coreclr/src/jit/gentree.h +++ b/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 diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index ee016bfe9e6a3..11146bac7a34d 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/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 = diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.cs b/src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.cs new file mode 100644 index 0000000000000..1bbbc9cdb903a --- /dev/null +++ b/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 + { + public S16 Foo(BigStruct b, int i, int j); + } + + public class CFoo : IFoo + { + public S16 Foo(BigStruct b, int i, int j) + { + S16 s16; + s16.a = (IntPtr)i; + s16.b = (uint)j; + return s16; + } + } + + class Test + { + IFoo m_if = new CFoo(); + 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; + } + } + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.csproj new file mode 100644 index 0000000000000..d6abcf55adaad --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_49078/Runtime_49078.csproj @@ -0,0 +1,14 @@ + + + Exe + 1 + + + true + None + True + + + + +