From b2e3f966d8c8502e4bed05574fac713b9e4fbe42 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 19 May 2021 15:42:41 -0700 Subject: [PATCH 1/2] JIT: fix invocation of unboxed entry when method returns struct If a value class method returns a struct, and its unboxed entry point requires a type context argument, make sure to pass the context argument properly. Also, fetch the type context (method table) from the box, rather than creating it from the class handle we have on hand; this tends to produce smaller code as we're often fetching the method table for other reasons anyways. Closes #52975. --- src/coreclr/jit/importer.cpp | 46 ++++++++++---- .../Devirtualization/structreturningstruct.cs | 63 +++++++++++++++++++ .../structreturningstruct.csproj | 27 ++++++++ 3 files changed, 123 insertions(+), 13 deletions(-) create mode 100644 src/tests/JIT/opt/Devirtualization/structreturningstruct.cs create mode 100644 src/tests/JIT/opt/Devirtualization/structreturningstruct.csproj diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index c7088274136562..213f7d7b64d5f4 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -21115,7 +21115,17 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, // if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) { - call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); + // If there's a ret buf, the context is the second arg. + // + if (call->HasRetBufArg()) + { + GenTreeCall::Use* const beforeArg = call->gtCallArgs; + beforeArg->SetNext(gtNewCallArgs(methodTableArg)); + } + else + { + call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); + } } else { @@ -21194,26 +21204,26 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, // locally, or was boxed locally but we were unable to remove the box for // various reasons. // - // We may still be able to update the call to invoke the unboxed entry. + // We can still to update the call to invoke the unboxed entry, if the + // boxed value is simple. // if (requiresInstMethodTableArg) { - const DWORD derivedClassAttribs = info.compCompHnd->getClassAttribs(derivedClass); + // Get the method table from the boxed object. + // + GenTree* const thisArg = call->gtCallThisArg->GetNode(); + GenTree* const clonedThisArg = gtClone(thisArg); - if ((derivedClassAttribs & CORINFO_FLG_SHAREDINST) != 0) + if (clonedThisArg == nullptr) { JITDUMP( - "unboxed entry needs MT arg, but the handle we have is shared. Deferring update.\n"); + "unboxed entry needs MT arg, but `this` was too complex to clone. Deferring update.\n"); } else { - // See if the method table we have is a viable argument to the unboxed - // entry point. It is, if it's not a shared MT. - // - GenTree* const thisArg = call->gtCallThisArg->GetNode(); - GenTree* const methodTableArg = gtNewIconEmbClsHndNode(derivedClass); + JITDUMP("revising call to invoke unboxed entry with additional method table arg\n"); - JITDUMP("revising call to invoke unboxed entry with additional known class handle arg\n"); + GenTree* const methodTableArg = gtNewMethodTableLookup(clonedThisArg); // Update the 'this' pointer to refer to the box payload // @@ -21232,14 +21242,24 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, derivedMethod = unboxedEntryMethod; derivedMethodAttribs = unboxedMethodAttribs; - // Add the method table argument... + // Add the method table argument. // // Prepend for R2L arg passing or empty L2R passing // Append for non-empty L2R // if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) { - call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); + // If there's a ret buf, the context is the second arg. + // + if (call->HasRetBufArg()) + { + GenTreeCall::Use* const beforeArg = call->gtCallArgs; + beforeArg->SetNext(gtNewCallArgs(methodTableArg)); + } + else + { + call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); + } } else { diff --git a/src/tests/JIT/opt/Devirtualization/structreturningstruct.cs b/src/tests/JIT/opt/Devirtualization/structreturningstruct.cs new file mode 100644 index 00000000000000..96a14e5e068b71 --- /dev/null +++ b/src/tests/JIT/opt/Devirtualization/structreturningstruct.cs @@ -0,0 +1,63 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections; +using System.Collections.Generic; +using System.Threading; + +// Runtime issue 52975. +// +// If we devirtualize an interface call on a struct, ** and ** +// update the call site to invoke the unboxed entry, ** and ** +// the method returns a struct via hidden buffer pointer, ** and ** +// the unboxed method requires a type context arg, +// +// we need to be careful to pass the type context argument +// in the right spot in the arglist. +// +// The test below is set up to devirtualize under PGO. +// +// COMPlus_TieredPGO=1 +// COMPlus_TC_QuickJitForLoopsO=1 +// +class X +{ + static int F(IDictionary i) + { + int r = 0; + IDictionaryEnumerator e = i.GetEnumerator(); + while (e.MoveNext()) + { + // This is the critical call. + // + DictionaryEntry entry = e.Entry; + r++; + } + return r; + } + + public static int Main() + { + Dictionary s = new Dictionary(); + s["hello"] = "world"; + int r = 0; + + for (int i = 0; i < 50; i++) + { + r += F(s); + Thread.Sleep(15); + } + + int iter = 100; + + for (int i = 0; i < iter; i++) + { + r += F(s); + } + + int result = 2 * (r - iter); + Console.WriteLine($"Result={result}"); + return result; + } +} diff --git a/src/tests/JIT/opt/Devirtualization/structreturningstruct.csproj b/src/tests/JIT/opt/Devirtualization/structreturningstruct.csproj new file mode 100644 index 00000000000000..1089e5d3dec23b --- /dev/null +++ b/src/tests/JIT/opt/Devirtualization/structreturningstruct.csproj @@ -0,0 +1,27 @@ + + + Exe + + + PdbOnly + True + + + + + + + + + + From ee006142b39966750c100523055d9742bb697584 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 20 May 2021 08:52:43 -0700 Subject: [PATCH 2/2] review feedback --- src/coreclr/jit/importer.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 213f7d7b64d5f4..b929424c560896 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -21115,12 +21115,11 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, // if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) { - // If there's a ret buf, the context is the second arg. + // If there's a ret buf, the method table is the second arg. // if (call->HasRetBufArg()) { - GenTreeCall::Use* const beforeArg = call->gtCallArgs; - beforeArg->SetNext(gtNewCallArgs(methodTableArg)); + gtInsertNewCallArgAfter(methodTableArg, call->gtCallArgs); } else { @@ -21204,7 +21203,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, // locally, or was boxed locally but we were unable to remove the box for // various reasons. // - // We can still to update the call to invoke the unboxed entry, if the + // We can still update the call to invoke the unboxed entry, if the // boxed value is simple. // if (requiresInstMethodTableArg) @@ -21249,12 +21248,11 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, // if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) { - // If there's a ret buf, the context is the second arg. + // If there's a ret buf, the method table is the second arg. // if (call->HasRetBufArg()) { - GenTreeCall::Use* const beforeArg = call->gtCallArgs; - beforeArg->SetNext(gtNewCallArgs(methodTableArg)); + gtInsertNewCallArgAfter(methodTableArg, call->gtCallArgs); } else {