From e0eda5f17b22fafb6cf704944ec5bbc4a3bbfa22 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 20 May 2021 11:56:11 -0700 Subject: [PATCH] JIT: fix invocation of unboxed entry when method returns struct (#52998) 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 | 44 +++++++++---- .../Devirtualization/structreturningstruct.cs | 63 +++++++++++++++++++ .../structreturningstruct.csproj | 27 ++++++++ 3 files changed, 121 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 c708827413656..b929424c56089 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -21115,7 +21115,16 @@ 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 method table is the second arg. + // + if (call->HasRetBufArg()) + { + gtInsertNewCallArgAfter(methodTableArg, call->gtCallArgs); + } + else + { + call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); + } } else { @@ -21194,26 +21203,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 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 +21241,23 @@ 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 method table is the second arg. + // + if (call->HasRetBufArg()) + { + gtInsertNewCallArgAfter(methodTableArg, call->gtCallArgs); + } + 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 0000000000000..96a14e5e068b7 --- /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 0000000000000..1089e5d3dec23 --- /dev/null +++ b/src/tests/JIT/opt/Devirtualization/structreturningstruct.csproj @@ -0,0 +1,27 @@ + + + Exe + + + PdbOnly + True + + + + + + + + + +