Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit 59be047

Browse files
erozenfelderozenfeld
committed
Fix fast tail call lowering bug.
Lowering::LowerFastTailCall has code that handles the case when the caller and the callee both have parameters passed on the stack and the callee has register arguments that are computed from the caller's stack-passed arguments. In that case the stack arguments of the callee are set up in the caller's incoming argument area so they override the caller's arguments. Lowering::LowerFastTailCall creates temps for any caller's arguments passed on the stack that are used to compute callee's register arguments. That code had a bug where it was converting GT_LCL_FLD nodes into GT_LCL_VAR nodes. That's problematic both for the case of a single-field struct with a double field and for multi-field structs. In the former case we were getting LSRA asserts in checked builds and SBCG in release builds. In the latter case we were getting SBCG both in checked and release builds. The fix is not to convert GT_LCL_FLD nodes into GT_LCL_VAR nodes and mark the new temps as not-enregisterable if the local is not-enregisterable. An alternative fix would be to create multiple register temps for each GT_LCL_FLD or GT_LCL_VAR targeting the local but that would complicate the code and I decided to go with a simlper solution for this uncommon case.
1 parent a474890 commit 59be047

File tree

3 files changed

+152
-5
lines changed

3 files changed

+152
-5
lines changed

‎src/jit/lower.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1723,14 +1723,14 @@ void Lowering::LowerFastTailCall(GenTreeCall* call)
17231723
{
17241724
tmpLclNum = comp->lvaGrabTemp(
17251725
true DEBUGARG("Fast tail call lowering is creating a new local variable"));
1726-
comp->lvaSortAgain = true;
1727-
tmpType = genActualType(callerArgDsc->lvaArgType());
1728-
comp->lvaTable[tmpLclNum].lvType = tmpType;
1729-
comp->lvaTable[tmpLclNum].lvRefCnt = 1;
1726+
comp->lvaSortAgain = true;
1727+
tmpType = genActualType(callerArgDsc->lvaArgType());
1728+
comp->lvaTable[tmpLclNum].lvType = tmpType;
1729+
comp->lvaTable[tmpLclNum].lvRefCnt = 1;
1730+
comp->lvaTable[tmpLclNum].lvDoNotEnregister = comp->lvaTable[lcl->gtLclNum].lvDoNotEnregister;
17301731
}
17311732

17321733
lcl->SetLclNum(tmpLclNum);
1733-
lcl->SetOper(GT_LCL_VAR);
17341734
}
17351735
}
17361736
}

‎tests/src/JIT/Regression/JitBlue/GitHub_12037/GitHub_12037.cs‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System;
6+
7+
public class TailCallOptTest
8+
{
9+
public static int Main()
10+
{
11+
bool res1 = Caller1(new object(), 0L, 0xBEEF, new TypedDouble(1.0), new TypedDouble(2.0), new TypedDouble(3.0));
12+
bool res2 = Caller2(new object(), 0L, 0xBEEF, new TypedDouble(1.0), new TwoInts(3, 5), new TypedDouble(3.0));
13+
return (res1 && res2) ? 100 : 0;
14+
}
15+
16+
// In this test typedDouble2 is passed to Caller1 on the stack. Then typedDouble2.Value is passed to Callee1 in a register.
17+
// Since Calee1 also has a stack argument (typedDouble3.Value) and the call is dispatched as a fast tail call,
18+
// it's set up in the incoming argument area of Caller1. JIT lowering code needs to ensure that typedDouble2
19+
// in that area is not overwritten by typedDouble3.Value before typedDouble2.Value is computed. The JIT had a bug in that code
20+
// because typedDouble2.Value was represented as GT_LCL_FLD and it was incorrectly converted into GT_LCL_VAR resulting in type
21+
// mismatches (long vs. double since the struct is passed as a long but its only field is double).
22+
public static bool Caller1(object parameters, long l,
23+
double doubleArg, TypedDouble typedDouble1, TypedDouble typedDouble2, TypedDouble typedDouble3)
24+
{
25+
double param = 19.0;
26+
27+
Console.Write("Let's ");
28+
Console.Write("Discourage ");
29+
Console.Write("Inlining ");
30+
Console.Write("Of ");
31+
Console.Write("Caller ");
32+
Console.Write("Into ");
33+
Console.WriteLine("Main.");
34+
35+
return Callee1(doubleArg, param, typedDouble1.Value, typedDouble2.Value, typedDouble3.Value);
36+
}
37+
38+
public static bool Callee1(double doubleArg, double param, double typedDoubleArg1, double typedDoubleArg2, double typedDoubleArg3)
39+
{
40+
Console.WriteLine("{0} {1} {2} {3} {4}", doubleArg, param, typedDoubleArg1, typedDoubleArg2, typedDoubleArg3);
41+
if ((doubleArg == 0xBEEF) && (param == 19.0) && (typedDoubleArg1 == 1.0) && (typedDoubleArg2 == 2.0) && (typedDoubleArg3 == 3.0))
42+
{
43+
Console.WriteLine("PASSED");
44+
return true;
45+
}
46+
else
47+
{
48+
Console.WriteLine("FAILED");
49+
return false;
50+
}
51+
}
52+
53+
// In this test twoInts is passed to Caller2 on the stack. Then twoInts.Value1 and twoInts.Value2 were passed to Callee2 in registers.
54+
// Since Calee2 also has a stack argument (i3) and the call is dispatched as a fast tail call,
55+
// it's set up in the incoming argument area of Caller2. JIT lowering code needs to ensure that twoInts
56+
// in that area is not overwritten by i3 before twoInts.Value1 and twoInts.Value2 are computed. The JIT had a bug in that code
57+
// because twoInts.Value1 and twoInts.Value2 were represented as GT_LCL_FLD and they were incorrectly converted into GT_LCL_VAR
58+
// resulting in an identical value passed for both fields.
59+
public static bool Caller2(object parameters, long l,
60+
double doubleArg, TypedDouble typedDouble1, TwoInts twoInts, TypedDouble typedDouble3)
61+
{
62+
double param = 19.0;
63+
64+
Console.Write("Let's ");
65+
Console.Write("Discourage ");
66+
Console.Write("Inlining ");
67+
Console.Write("Of ");
68+
Console.Write("Caller ");
69+
Console.Write("Into ");
70+
Console.WriteLine("Main.");
71+
72+
return Callee2(twoInts.Value1, twoInts.Value2, typedDouble1.Value, param, 11);
73+
}
74+
75+
public static bool Callee2(int i1, int i2, double typedDoubleArg1, double typedDoubleArg2, int i3)
76+
{
77+
Console.WriteLine("{0} {1} {2} {3} {4}", i1, i2, typedDoubleArg1, typedDoubleArg2, i3);
78+
if ((i1 == 3) && (i2 == 5) && (typedDoubleArg1 == 1.0) && (typedDoubleArg2 == 19) && (i3 == 11))
79+
{
80+
Console.WriteLine("PASSED");
81+
return true;
82+
}
83+
else
84+
{
85+
Console.WriteLine("FAILED");
86+
return false;
87+
}
88+
}
89+
90+
public struct TypedDouble
91+
{
92+
public TypedDouble(double value)
93+
{
94+
Value = value;
95+
}
96+
public readonly double Value;
97+
}
98+
99+
public struct TwoInts
100+
{
101+
public TwoInts(int value1, int value2)
102+
{
103+
Value1 = value1;
104+
Value2 = value2;
105+
}
106+
public readonly int Value1;
107+
public readonly int Value2;
108+
}
109+
}

‎tests/src/JIT/Regression/JitBlue/GitHub_12037/GitHub_12037.csproj‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<?xml version="1.0" encoding="utf-8"?>
2+
<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
3+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
4+
<PropertyGroup>
5+
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
6+
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
7+
<AssemblyName>$(MSBuildProjectName)</AssemblyName>
8+
<SchemaVersion>2.0</SchemaVersion>
9+
<ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid>
10+
<OutputType>Exe</OutputType>
11+
<ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
12+
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
13+
14+
</PropertyGroup>
15+
<!-- Default configurations to help VS understand the configurations -->
16+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
17+
</PropertyGroup>
18+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
19+
</PropertyGroup>
20+
<ItemGroup>
21+
<CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
22+
<Visible>False</Visible>
23+
</CodeAnalysisDependentAssemblyPaths>
24+
</ItemGroup>
25+
<PropertyGroup>
26+
<DebugType></DebugType>
27+
<Optimize>True</Optimize>
28+
</PropertyGroup>
29+
<ItemGroup>
30+
<Compile Include="$(MSBuildProjectName).cs" />
31+
</ItemGroup>
32+
<ItemGroup>
33+
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
34+
</ItemGroup>
35+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
36+
<PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
37+
</PropertyGroup>
38+
</Project>

0 commit comments

Comments
 (0)