Skip to content

Commit

Permalink
JIT: fix invocation of unboxed entry when method returns struct (#52998)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
AndyAyersMS committed May 20, 2021
1 parent b3c10ee commit e0eda5f
Show file tree
Hide file tree
Showing 3 changed files with 121 additions and 13 deletions.
44 changes: 31 additions & 13 deletions src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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
//
Expand All @@ -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
{
Expand Down
63 changes: 63 additions & 0 deletions src/tests/JIT/opt/Devirtualization/structreturningstruct.cs
Original file line number Diff line number Diff line change
@@ -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<string, string> s = new Dictionary<string, string>();
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;
}
}
27 changes: 27 additions & 0 deletions src/tests/JIT/opt/Devirtualization/structreturningstruct.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
</PropertyGroup>
<PropertyGroup>
<DebugType>PdbOnly</DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<PropertyGroup>
<!-- This test requires tiered compilation and PGO -->
<CLRTestBatchPreCommands><![CDATA[
$(CLRTestBatchPreCommands)
set DOTNET_TieredCompilation=1
set DOTNET_TieredPGO=1
set DOTNET_TC_QuickJitForLoops=1
]]></CLRTestBatchPreCommands>
<BashCLRTestPreCommands><![CDATA[
$(BashCLRTestPreCommands)
export DOTNET_TieredCompilation=1
export DOTNET_TieredPGO=1
export DOTNET_TC_QuickJitForLoops=1
]]></BashCLRTestPreCommands>
</PropertyGroup>
<ItemGroup>
<Compile Include="structreturningstruct.cs" />
</ItemGroup>
</Project>

0 comments on commit e0eda5f

Please sign in to comment.