Skip to content

Commit

Permalink
[release/6.0] JIT: Avoid removing multi-use boxes (#73014)
Browse files Browse the repository at this point in the history
* JIT: Avoid removing multi-use boxes

There was an assumption that the local operand to a GT_BOX node is
single-use which was being violated when GDV clones these as arguments.
We now allow multi-uses of these locals by setting a flag when cloning
and then handle it in gtTryRemoveBoxUpstreamEffects.

There is also GTF_VAR_CLONED that would be set on the local itself, but
given that transformations can affect the operand local node arbitrarily
I went with another of these type of flags on GT_BOX instead.

Fix #72775

* Add a test

* Test nit

Co-authored-by: Jakob Botsch Nielsen <jakob.botsch.nielsen@gmail.com>
  • Loading branch information
github-actions[bot] and jakobbotsch committed Aug 11, 2022
1 parent b01798f commit 708d716
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 0 deletions.
5 changes: 5 additions & 0 deletions src/coreclr/jit/compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9175,6 +9175,11 @@ void cTreeFlags(Compiler* comp, GenTree* tree)
{
chars += printf("[BOX_VALUE]");
}

if (tree->gtFlags & GTF_BOX_CLONED)
{
chars += printf("[BOX_CLONED]");
}
break;

case GT_CNS_INT:
Expand Down
9 changes: 9 additions & 0 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7935,6 +7935,8 @@ GenTree* Compiler::gtCloneExpr(
copy = new (this, GT_BOX)
GenTreeBox(tree->TypeGet(), tree->AsOp()->gtOp1, tree->AsBox()->gtAsgStmtWhenInlinedBoxValue,
tree->AsBox()->gtCopyStmtWhenInlinedBoxValue);
tree->AsBox()->SetCloned();
copy->AsBox()->SetCloned();
break;

case GT_INTRINSIC:
Expand Down Expand Up @@ -13834,6 +13836,13 @@ GenTree* Compiler::gtTryRemoveBoxUpstreamEffects(GenTree* op, BoxRemovalOptions
return nullptr;
}

// If this box is no longer single-use, bail.
if (box->WasCloned())
{
JITDUMP(" bailing; unsafe to remove box that has been cloned\n");
return nullptr;
}

// If we're eventually going to return the type handle, remember it now.
GenTree* boxTypeHandle = nullptr;
if ((options == BR_REMOVE_AND_NARROW_WANT_TYPE_HANDLE) || (options == BR_DONT_REMOVE_WANT_TYPE_HANDLE))
Expand Down
11 changes: 11 additions & 0 deletions src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,7 @@ enum GenTreeFlags : unsigned int
GTF_QMARK_CAST_INSTOF = 0x80000000, // GT_QMARK -- Is this a top (not nested) level qmark created for
// castclass or instanceof?

GTF_BOX_CLONED = 0x40000000, // GT_BOX -- this box and its operand has been cloned, cannot assume it to be single-use anymore
GTF_BOX_VALUE = 0x80000000, // GT_BOX -- "box" is on a value type

GTF_ICON_HDL_MASK = 0xF0000000, // Bits used by handle types below
Expand Down Expand Up @@ -3638,6 +3639,16 @@ struct GenTreeBox : public GenTreeUnOp
{
}
#endif

bool WasCloned()
{
return (gtFlags & GTF_BOX_CLONED) != 0;
}

void SetCloned()
{
gtFlags |= GTF_BOX_CLONED;
}
};

/* gtField -- data member ref (GT_FIELD) */
Expand Down
53 changes: 53 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_72775/Runtime_72775.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// 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.Runtime.CompilerServices;
using System.Threading;

public class Runtime_72775
{
public static int Main()
{
for (int i = 0; i < 100; i++)
{
Call(new Impl1());
if (i > 30 && i < 40)
Thread.Sleep(10);
}

// With GDV, JIT would optimize Call by fully removing the box since Impl1.Foo does not use it.
// This would cause null to be passed to Impl2.Foo.
return Call(new Impl2());
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static int Call(I i) => i.Foo(5);
}

public interface I
{
int Foo(object o);
}

class Impl1 : I
{
public int Foo(object o) => 0;
}

class Impl2 : I
{
public int Foo(object o)
{
if (o is not int i || i != 5)
{
Console.WriteLine("FAIL: Got {0}", o?.ToString() ?? "(null)");
return -1;
}
else
{
Console.WriteLine("PASS: Got 5");
return 100;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
<PropertyGroup>
<CLRTestBatchPreCommands><![CDATA[
$(CLRTestBatchPreCommands)
set COMPlus_TieredCompilation=1
set COMPlus_TieredPGO=1
]]></CLRTestBatchPreCommands>
<BashCLRTestPreCommands><![CDATA[
$(BashCLRTestPreCommands)
export COMPlus_TieredCompilation=1
export COMPlus_TieredPGO=1
]]></BashCLRTestPreCommands>
</PropertyGroup>
</Project>

0 comments on commit 708d716

Please sign in to comment.