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

Commit ad48067

Browse files
CarolEidtCarolEidt
committed
ARM: Fix morphing of struct passed on stack
If a struct is passed on the stack, it must live on the stack, unless/until we support `GT_FIELD_LIST` for these args. This is unlikely to represent a significant code quality issue, since ARM supports many register args, and this has gone undetected thus far. This was exposed by tailcall stress on desktop. I've added a test that exposes the issue without tailcall stress (though it gets a different assert than the desktop failure). It seemed that `fgMorphMultiregStructArg()` was the best place to fix this - and I noted that this is called for any struct that is larger than a single register. So I updated the comments to reflect that. I thought about putting the test in the JIT\Regressions test directory, but I consider that it is addressing basic missing test coverage, so I added it to JIT\Methodical\structs.
1 parent 66bd34e commit ad48067

File tree

3 files changed

+265
-21
lines changed

3 files changed

+265
-21
lines changed

‎src/jit/morph.cpp‎

Lines changed: 41 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3297,7 +3297,10 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call)
32973297
SYSTEMV_AMD64_CORINFO_STRUCT_REG_PASSING_DESCRIPTOR structDesc;
32983298
#endif // FEATURE_UNIX_AMD64_STRUCT_PASSING
32993299

3300-
bool hasStructArgument = false; // @TODO-ARM64-UNIX: Remove this bool during a future refactoring
3300+
bool hasStructArgument = false; // @TODO-ARM64-UNIX: Remove this bool during a future refactoring
3301+
// hasMultiregStructArgs is true if there are any structs that are eligible for passing
3302+
// in registers; this is true even if it is not actually passed in registers (i.e. because
3303+
// previous arguments have used up available argument registers).
33013304
bool hasMultiregStructArgs = false;
33023305
for (args = call->gtCallArgs; args; args = args->gtOp.gtOp2, argIndex++)
33033306
{
@@ -3557,7 +3560,8 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call)
35573560

35583561
if (size == 2)
35593562
{
3560-
// Structs that are the size of 2 pointers are passed by value in multiple registers
3563+
// Structs that are the size of 2 pointers are passed by value in multiple registers,
3564+
// if sufficient registers are available.
35613565
hasMultiregStructArgs = true;
35623566
}
35633567
else if (size > 2)
@@ -4594,7 +4598,8 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call)
45944598
// In the future we can migrate UNIX_AMD64 to use this
45954599
// method instead of fgMorphSystemVStructArgs
45964600

4597-
// We only build GT_FIELD_LISTs for MultiReg structs for the RyuJIT backend
4601+
// We only require morphing of structs that may be passed in multiple registers
4602+
// for the RyuJIT backend.
45984603
if (hasMultiregStructArgs)
45994604
{
46004605
fgMorphMultiregStructArgs(call);
@@ -4808,9 +4813,8 @@ void Compiler::fgMorphSystemVStructArgs(GenTreeCall* call, bool hasStructArgumen
48084813
// call: a GenTreeCall node that has one or more TYP_STRUCT arguments
48094814
//
48104815
// Notes:
4811-
// We only call fgMorphMultiregStructArg for the register passed TYP_STRUCT arguments.
4812-
// The call to fgMorphMultiregStructArg will mutate the argument into the GT_FIELD_LIST form
4813-
// which is only used for struct arguments.
4816+
// We only call fgMorphMultiregStructArg for struct arguments that are not passed as simple types.
4817+
// It will ensure that the struct arguments are in the correct form.
48144818
// If this method fails to find any TYP_STRUCT arguments it will assert.
48154819
//
48164820
void Compiler::fgMorphMultiregStructArgs(GenTreeCall* call)
@@ -4900,17 +4904,21 @@ void Compiler::fgMorphMultiregStructArgs(GenTreeCall* call)
49004904
}
49014905

49024906
//-----------------------------------------------------------------------------
4903-
// fgMorphMultiregStructArg: Given a multireg TYP_STRUCT arg from a call argument list
4904-
// Morph the argument into a set of GT_FIELD_LIST nodes.
4907+
// fgMorphMultiregStructArg: Given a TYP_STRUCT arg from a call argument list,
4908+
// morph the argument as needed to be passed correctly.
49054909
//
49064910
// Arguments:
4907-
// arg - A GenTree node containing a TYP_STRUCT arg that
4908-
// is to be passed in multiple registers
4911+
// arg - A GenTree node containing a TYP_STRUCT arg
49094912
// fgEntryPtr - the fgArgTabEntry information for the current 'arg'
49104913
//
49114914
// Notes:
4912-
// arg must be a GT_OBJ or GT_LCL_VAR or GT_LCL_FLD of TYP_STRUCT that is suitable
4913-
// for passing in multiple registers.
4915+
// The arg must be a GT_OBJ or GT_LCL_VAR or GT_LCL_FLD of TYP_STRUCT.
4916+
// If 'arg' is a lclVar passed on the stack, we will ensure that any lclVars that must be on the
4917+
// stack are marked as doNotEnregister, and then we return.
4918+
//
4919+
// If it is passed by register, we mutate the argument into the GT_FIELD_LIST form
4920+
// which is only used for struct arguments.
4921+
//
49144922
// If arg is a LclVar we check if it is struct promoted and has the right number of fields
49154923
// and if they are at the appropriate offsets we will use the struct promted fields
49164924
// in the GT_FIELD_LIST nodes that we create.
@@ -4933,22 +4941,34 @@ GenTreePtr Compiler::fgMorphMultiregStructArg(GenTreePtr arg, fgArgTabEntryPtr f
49334941
if ((fgEntryPtr->isSplit && fgEntryPtr->numSlots + fgEntryPtr->numRegs > 4) ||
49344942
(!fgEntryPtr->isSplit && fgEntryPtr->regNum == REG_STK))
49354943
{
4944+
GenTreeLclVarCommon* lcl = nullptr;
4945+
49364946
// If already OBJ it is set properly already.
49374947
if (arg->OperGet() == GT_OBJ)
49384948
{
4939-
return arg;
4949+
if (arg->gtGetOp1()->OperIs(GT_ADDR) && arg->gtGetOp1()->gtGetOp1()->OperIs(GT_LCL_VAR))
4950+
{
4951+
lcl = arg->gtGetOp1()->gtGetOp1()->AsLclVarCommon();
4952+
}
49404953
}
4954+
else
4955+
{
4956+
assert(arg->OperGet() == GT_LCL_VAR);
49414957

4942-
assert(arg->OperGet() == GT_LCL_VAR);
4943-
4944-
// We need to construct a `GT_OBJ` node for the argmuent,
4945-
// so we need to get the address of the lclVar.
4946-
GenTreeLclVarCommon* lclCommon = arg->AsLclVarCommon();
4958+
// We need to construct a `GT_OBJ` node for the argmuent,
4959+
// so we need to get the address of the lclVar.
4960+
lcl = arg->AsLclVarCommon();
49474961

4948-
arg = gtNewOperNode(GT_ADDR, TYP_I_IMPL, arg);
4962+
arg = gtNewOperNode(GT_ADDR, TYP_I_IMPL, arg);
49494963

4950-
// Create an Obj of the temp to use it as a call argument.
4951-
arg = gtNewObjNode(lvaGetStruct(lclCommon->gtLclNum), arg);
4964+
// Create an Obj of the temp to use it as a call argument.
4965+
arg = gtNewObjNode(lvaGetStruct(lcl->gtLclNum), arg);
4966+
}
4967+
if (lcl != nullptr)
4968+
{
4969+
// Its fields will need to accessed by address.
4970+
lvaSetVarDoNotEnregister(lcl->gtLclNum, DNER_IsStructArg);
4971+
}
49524972

49534973
return arg;
49544974
}

‎tests/src/JIT/Methodical/structs/StructStackParams.cs‎

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
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+
6+
// This tests passing structs that are less than 64-bits in size, but that
7+
// don't match the size of a primitive type, and passes them as the 6th
8+
// parameter so that they are likely to wind up on the stack for ABIs that
9+
// pass structs by value.
10+
11+
using System;
12+
using System.Runtime.CompilerServices;
13+
14+
// Struct that's greater than 32-bits, but not a multiple of 32-bits.
15+
public struct MyStruct1
16+
{
17+
public byte f1;
18+
public byte f2;
19+
public short f3;
20+
public short f4;
21+
}
22+
23+
// Struct that's less than 32-bits, but not the same size as any primitive type.
24+
public struct MyStruct2
25+
{
26+
public byte f1;
27+
public byte f2;
28+
public byte f3;
29+
}
30+
31+
// Struct that's less than 64-bits, but not the same size as any primitive type.
32+
public struct MyStruct3
33+
{
34+
public short f1;
35+
public short f2;
36+
public short f3;
37+
}
38+
39+
// Struct that's greater than 64-bits, but not a multiple of 64-bits.
40+
public struct MyStruct4
41+
{
42+
public int f1;
43+
public int f2;
44+
public short f3;
45+
}
46+
47+
public class MyProgram
48+
{
49+
const int Pass = 100;
50+
const int Fail = -1;
51+
52+
[MethodImplAttribute(MethodImplOptions.NoInlining)]
53+
public static byte GetByte(byte i)
54+
{
55+
return i;
56+
}
57+
58+
[MethodImplAttribute(MethodImplOptions.NoInlining)]
59+
public static short GetShort(short i)
60+
{
61+
return i;
62+
}
63+
64+
[MethodImplAttribute(MethodImplOptions.NoInlining)]
65+
public static int GetInt(int i)
66+
{
67+
return i;
68+
}
69+
70+
[MethodImplAttribute(MethodImplOptions.NoInlining)]
71+
public static int Check1(int w, int i1, int i2, int i3, int i4, int i5, int i6, int i7, MyStruct1 s1)
72+
{
73+
if ((w != 1) || (s1.f1 != i1) || (s1.f2 != i2) || (s1.f3 != i3) || (s1.f4 != i4))
74+
{
75+
Console.WriteLine("FAIL");
76+
return Fail;
77+
}
78+
Console.WriteLine("PASS");
79+
return Pass;
80+
}
81+
82+
public static int TestStruct1()
83+
{
84+
MyStruct1 s1;
85+
s1.f1 = GetByte(1); s1.f2 = GetByte(2); s1.f3 = GetShort(3); s1.f4 = GetShort(4);
86+
int x = (s1.f1 * s1.f2 * s1.f3 * s1.f4);
87+
int y = (s1.f1 - s1.f2) * (s1.f3 - s1.f4);
88+
int z = (s1.f1 + s1.f2) * (s1.f3 + s1.f4);
89+
int w = (x + y) / z;
90+
91+
return Check1(w, 1, 2, 3, 4, 5, 6, 7, s1);
92+
}
93+
94+
[MethodImplAttribute(MethodImplOptions.NoInlining)]
95+
public static int Check2(int w, int i1, int i2, int i3, int i4, int i5, int i6, int i7, MyStruct2 s2)
96+
{
97+
if ((w != 2) || (s2.f1 != i1) || (s2.f2 != i2) || (s2.f3 != i3) || (i4 != 4))
98+
{
99+
Console.WriteLine("FAIL");
100+
return Fail;
101+
}
102+
Console.WriteLine("PASS");
103+
return Pass;
104+
}
105+
106+
public static int TestStruct2()
107+
{
108+
MyStruct2 s2;
109+
s2.f1 = GetByte(1); s2.f2 = GetByte(2); s2.f3 = GetByte(3);
110+
int x = s2.f1 * s2.f2 * s2.f3;
111+
int y = (s2.f1 + s2.f2) * s2.f3;
112+
int z = s2.f1 + s2.f2 + s2.f3;
113+
int w = (x + y) / z;
114+
115+
return Check2(w, 1, 2, 3, 4, 5, 6, 7, s2);
116+
}
117+
118+
[MethodImplAttribute(MethodImplOptions.NoInlining)]
119+
public static int Check3(int w, int i1, int i2, int i3, int i4, int i5, int i6, int i7, MyStruct3 s3)
120+
{
121+
if ((w != 2) || (s3.f1 != i1) || (s3.f2 != i2) || (s3.f3 != i3) || (i4 != 4))
122+
{
123+
Console.WriteLine("FAIL");
124+
return Fail;
125+
}
126+
Console.WriteLine("PASS");
127+
return Pass;
128+
}
129+
130+
public static int TestStruct3()
131+
{
132+
MyStruct3 s3;
133+
s3.f1 = GetByte(1); s3.f2 = GetByte(2); s3.f3 = GetByte(3);
134+
int x = s3.f1 * s3.f2 * s3.f3;
135+
int y = (s3.f1 + s3.f2) * s3.f3;
136+
int z = s3.f1 + s3.f2 + s3.f3;
137+
int w = (x + y) / z;
138+
139+
return Check3(w, 1, 2, 3, 4, 5, 6, 7, s3);
140+
}
141+
142+
[MethodImplAttribute(MethodImplOptions.NoInlining)]
143+
public static int Check4(int w, int i1, int i2, int i3, int i4, int i5, int i6, int i7, MyStruct4 s4)
144+
{
145+
if ((w != 2) || (s4.f1 != i1) || (s4.f2 != i2) || (s4.f3 != i3) || (i4 != 4))
146+
{
147+
Console.WriteLine("FAIL");
148+
return Fail;
149+
}
150+
Console.WriteLine("PASS");
151+
return Pass;
152+
}
153+
154+
public static int TestStruct4()
155+
{
156+
MyStruct4 s4;
157+
s4.f1 = GetInt(1); s4.f2 = GetInt(2); s4.f3 = GetShort(3);
158+
int x = s4.f1 * s4.f2 * s4.f3;
159+
int y = (s4.f1 + s4.f2) * s4.f3;
160+
int z = s4.f1 + s4.f2 + s4.f3;
161+
int w = (x + y) / z;
162+
163+
return Check4(w, 1, 2, 3, 4, 5, 6, 7, s4);
164+
}
165+
166+
public static int Main()
167+
{
168+
int retVal = Pass;
169+
if (TestStruct1() != Pass)
170+
{
171+
retVal = Fail;
172+
}
173+
if (TestStruct2() != Pass)
174+
{
175+
retVal = Fail;
176+
}
177+
if (TestStruct3() != Pass)
178+
{
179+
retVal = Fail;
180+
}
181+
if (TestStruct4() != Pass)
182+
{
183+
retVal = Fail;
184+
}
185+
return retVal;
186+
}
187+
}

‎tests/src/JIT/Methodical/structs/StructStackParams.csproj‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
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+
</PropertyGroup>
14+
<!-- Default configurations to help VS understand the configurations -->
15+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
16+
</PropertyGroup>
17+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
18+
</PropertyGroup>
19+
<ItemGroup>
20+
<CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
21+
<Visible>False</Visible>
22+
</CodeAnalysisDependentAssemblyPaths>
23+
</ItemGroup>
24+
<PropertyGroup>
25+
<DebugType></DebugType>
26+
<Optimize>True</Optimize>
27+
</PropertyGroup>
28+
<ItemGroup>
29+
<Compile Include="$(MSBuildProjectName).cs" />
30+
</ItemGroup>
31+
<ItemGroup>
32+
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
33+
</ItemGroup>
34+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
35+
<PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
36+
</PropertyGroup>
37+
</Project>

0 commit comments

Comments
 (0)