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

Commit 26a2e19

Browse files
author
Sergey Andreenko
authored
[RyuJit/ARM] Fix lsra BuildShiftRotate (#17103)
* add repro * delete BuildShiftRotate for arm * fix GT_LSH_HI and GT_RSH_LO * return the special handling for GT_LSH_HI and GT_RSH_LO * fix the header
1 parent 28b7201 commit 26a2e19

File tree

6 files changed

+200
-69
lines changed

6 files changed

+200
-69
lines changed

‎src/jit/lsra.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1654,9 +1654,14 @@ class LinearScan : public LinearScanInterface
16541654

16551655
void BuildStoreLoc(GenTree* tree);
16561656
void BuildReturn(GenTree* tree);
1657+
#ifdef _TARGET_XARCH_
16571658
// This method, unlike the others, returns the number of sources, since it may be called when
16581659
// 'tree' is contained.
16591660
int BuildShiftRotate(GenTree* tree);
1661+
#endif // _TARGET_XARCH_
1662+
#ifdef _TARGET_ARM_
1663+
void BuildShiftLongCarry(GenTree* tree);
1664+
#endif
16601665
void BuildPutArgReg(GenTreeUnOp* node);
16611666
void BuildCall(GenTreeCall* call);
16621667
void BuildCmp(GenTree* tree);

‎src/jit/lsraarm.cpp‎

Lines changed: 51 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,46 @@ void LinearScan::BuildLclHeap(GenTree* tree)
112112
}
113113
}
114114

115+
//------------------------------------------------------------------------
116+
// BuildShiftLongCarry: Set the node info for GT_LSH_HI or GT_RSH_LO.
117+
//
118+
// Arguments:
119+
// tree - The node of interest
120+
//
121+
// Note: these operands have uses that interfere with the def and need the special handling.
122+
//
123+
void LinearScan::BuildShiftLongCarry(GenTree* tree)
124+
{
125+
assert(tree->OperGet() == GT_LSH_HI || tree->OperGet() == GT_RSH_LO);
126+
127+
GenTree* source = tree->gtOp.gtOp1;
128+
assert((source->OperGet() == GT_LONG) && source->isContained());
129+
130+
TreeNodeInfo* info = currentNodeInfo;
131+
info->srcCount = 2;
132+
133+
LocationInfoListNode* sourceLoInfo = getLocationInfo(source->gtOp.gtOp1);
134+
LocationInfoListNode* sourceHiInfo = getLocationInfo(source->gtOp.gtOp2);
135+
if (tree->OperGet() == GT_LSH_HI)
136+
{
137+
sourceLoInfo->info.isDelayFree = true;
138+
}
139+
else
140+
{
141+
sourceHiInfo->info.isDelayFree = true;
142+
}
143+
useList.Append(sourceLoInfo);
144+
useList.Append(sourceHiInfo);
145+
info->hasDelayFreeSrc = true;
146+
147+
GenTree* shiftBy = tree->gtOp.gtOp2;
148+
if (!shiftBy->isContained())
149+
{
150+
appendLocationInfoToList(shiftBy);
151+
info->srcCount += 1;
152+
}
153+
}
154+
115155
//------------------------------------------------------------------------
116156
// BuildNode: Set the register requirements for RA.
117157
//
@@ -335,11 +375,22 @@ void LinearScan::BuildNode(GenTree* tree)
335375
case GT_AND:
336376
case GT_OR:
337377
case GT_XOR:
378+
case GT_LSH:
379+
case GT_RSH:
380+
case GT_RSZ:
381+
case GT_ROR:
338382
assert(info->dstCount == 1);
339383
info->srcCount = appendBinaryLocationInfoToList(tree->AsOp());
340384
assert(info->srcCount == (tree->gtOp.gtOp2->isContained() ? 1 : 2));
341385
break;
342386

387+
case GT_LSH_HI:
388+
case GT_RSH_LO:
389+
assert(info->dstCount == 1);
390+
BuildShiftLongCarry(tree);
391+
assert(info->srcCount == (tree->gtOp.gtOp2->isContained() ? 2 : 3));
392+
break;
393+
343394
case GT_RETURNTRAP:
344395
// this just turns into a compare of its child with an int
345396
// + a conditional call
@@ -553,15 +604,6 @@ void LinearScan::BuildNode(GenTree* tree)
553604
appendLocationInfoToList(tree->gtOp.gtOp1);
554605
break;
555606

556-
case GT_LSH:
557-
case GT_RSH:
558-
case GT_RSZ:
559-
case GT_ROR:
560-
case GT_LSH_HI:
561-
case GT_RSH_LO:
562-
BuildShiftRotate(tree);
563-
break;
564-
565607
case GT_EQ:
566608
case GT_NE:
567609
case GT_LT:

‎src/jit/lsraarm64.cpp‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,10 @@ void LinearScan::BuildNode(GenTree* tree)
219219
case GT_AND:
220220
case GT_OR:
221221
case GT_XOR:
222+
case GT_LSH:
223+
case GT_RSH:
224+
case GT_RSZ:
225+
case GT_ROR:
222226
info->srcCount = appendBinaryLocationInfoToList(tree->AsOp());
223227
assert(info->dstCount == 1);
224228
break;
@@ -341,13 +345,6 @@ void LinearScan::BuildNode(GenTree* tree)
341345
assert(info->dstCount == 1);
342346
break;
343347

344-
case GT_LSH:
345-
case GT_RSH:
346-
case GT_RSZ:
347-
case GT_ROR:
348-
BuildShiftRotate(tree);
349-
break;
350-
351348
case GT_EQ:
352349
case GT_NE:
353350
case GT_LT:

‎src/jit/lsraarmarch.cpp‎

Lines changed: 0 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -111,59 +111,6 @@ void LinearScan::BuildIndir(GenTreeIndir* indirTree)
111111
#endif // FEATURE_SIMD
112112
}
113113

114-
//------------------------------------------------------------------------
115-
// BuildShiftRotate: Set the NodeInfo for a shift or rotate.
116-
//
117-
// Arguments:
118-
// tree - The node of interest
119-
//
120-
// Return Value:
121-
// None.
122-
//
123-
int LinearScan::BuildShiftRotate(GenTree* tree)
124-
{
125-
TreeNodeInfo* info = currentNodeInfo;
126-
GenTree* source = tree->gtOp.gtOp1;
127-
GenTree* shiftBy = tree->gtOp.gtOp2;
128-
assert(info->dstCount == 1);
129-
if (!shiftBy->isContained())
130-
{
131-
appendLocationInfoToList(shiftBy);
132-
info->srcCount = 1;
133-
}
134-
135-
#ifdef _TARGET_ARM_
136-
137-
// The first operand of a GT_LSH_HI and GT_RSH_LO oper is a GT_LONG so that
138-
// we can have a three operand form. Increment the srcCount.
139-
if (tree->OperGet() == GT_LSH_HI || tree->OperGet() == GT_RSH_LO)
140-
{
141-
assert((source->OperGet() == GT_LONG) && source->isContained());
142-
info->srcCount += 2;
143-
144-
LocationInfoListNode* sourceLoInfo = getLocationInfo(source->gtOp.gtOp1);
145-
useList.Append(sourceLoInfo);
146-
LocationInfoListNode* sourceHiInfo = getLocationInfo(source->gtOp.gtOp2);
147-
useList.Append(sourceHiInfo);
148-
if (tree->OperGet() == GT_LSH_HI)
149-
{
150-
sourceLoInfo->info.isDelayFree = true;
151-
}
152-
else
153-
{
154-
sourceHiInfo->info.isDelayFree = true;
155-
}
156-
info->hasDelayFreeSrc = true;
157-
}
158-
else
159-
#endif // _TARGET_ARM_
160-
{
161-
appendLocationInfoToList(source);
162-
info->srcCount++;
163-
}
164-
return info->srcCount;
165-
}
166-
167114
//------------------------------------------------------------------------
168115
// BuildCall: Set the NodeInfo for a call.
169116
//

‎tests/src/JIT/Regression/JitBlue/DevDiv_545497/DevDiv_545497.il‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
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+
// The bug that this test captures was a case where arm LSRA built wrong uses order for
6+
// the IL_0090 shift operand.
7+
8+
.assembly extern System.Runtime { auto }
9+
.assembly DevDiv_524309 {}
10+
11+
.class private auto ansi beforefieldinit DevDiv_545497
12+
extends [System.Runtime]System.Object
13+
{
14+
.method private hidebysig static uint8
15+
ILGEN_METHOD(float32 a,
16+
uint32 b,
17+
uint64 c) cil managed
18+
{
19+
.maxstack 194
20+
.locals init (float64, bool, unsigned int64)
21+
IL_0000: ldloc.s 0x01
22+
IL_0002: conv.r.un
23+
IL_0028: conv.ovf.u8.un
24+
IL_002c: ldloc.s 0x00
25+
IL_002e: conv.ovf.i8.un
26+
IL_002f: stloc 0x0002
27+
IL_0033: ldarg 0x0002
28+
IL_003f: ldloc 0x0001
29+
IL_0043: ldloc.s 0x01
30+
IL_0045: shl
31+
IL_0047: shr.un
32+
IL_0049: ldarg.s 0x01
33+
IL_005b: conv.i1
34+
IL_005d: conv.ovf.u
35+
IL_005e: ldloc 0x0001
36+
IL_0063: ldc.i4 0
37+
IL_0071: ldloc 0x0002
38+
IL_007a: conv.ovf.i1.un
39+
IL_007b: stloc 0x0001
40+
IL_007f: conv.i1
41+
IL_0081: rem.un
42+
IL_0082: ldarg 0x0002
43+
IL_0087: dup
44+
IL_0088: add.ovf.un
45+
IL_008a: ldloc.s 0x01
46+
IL_008c: ldloc 0x0001
47+
IL_0090: shr // This shift moves loc0x01 >> loc0x01 and exposed the original issue.
48+
IL_0091: conv.ovf.i8
49+
IL_0092: mul
50+
IL_0093: ldloc.s 0x01
51+
IL_0095: ldarg.s 0x01
52+
IL_0097: div.un
53+
IL_0098: starg 0x0001
54+
IL_009c: ldloc 0x0002
55+
IL_00b0: clt
56+
IL_00b2: conv.ovf.i1.un
57+
IL_00b3: sub.ovf.un
58+
IL_00b7: sub.ovf
59+
IL_00b9: shr.un
60+
IL_00ba: ldloc.s 0x01
61+
IL_00bc: conv.ovf.u8.un
62+
IL_00bd: and
63+
IL_00bf: clt.un
64+
IL_00c1: ret
65+
} // end of method DevDiv_545497::ILGEN_METHOD
66+
67+
.method private hidebysig static int32
68+
Main() cil managed
69+
{
70+
.entrypoint
71+
// Code size 31 (0x1f)
72+
.maxstack 3
73+
.locals init (int32 V_0)
74+
IL_0000: nop
75+
.try
76+
{
77+
IL_0001: nop
78+
IL_0002: ldc.r4 0.0
79+
IL_0007: ldc.i4.0
80+
IL_0008: ldc.i4.0
81+
IL_0009: conv.i8
82+
IL_000a: call uint8 DevDiv_545497::ILGEN_METHOD(float32,
83+
uint32,
84+
uint64)
85+
IL_000f: pop
86+
IL_0010: nop
87+
IL_0011: leave.s IL_0018
88+
89+
} // end .try
90+
catch [System.Runtime]System.Object
91+
{
92+
IL_0013: pop
93+
IL_0014: nop
94+
IL_0015: nop
95+
IL_0016: leave.s IL_0018
96+
97+
} // end handler
98+
IL_0018: ldc.i4.s 100
99+
IL_001a: stloc.0
100+
IL_001b: br.s IL_001d
101+
102+
IL_001d: ldloc.0
103+
IL_001e: ret
104+
} // end of method DevDiv_545497::Main
105+
106+
.method public hidebysig specialname rtspecialname
107+
instance void .ctor() cil managed
108+
{
109+
// Code size 8 (0x8)
110+
.maxstack 8
111+
IL_0000: ldarg.0
112+
IL_0001: call instance void [System.Runtime]System.Object::.ctor()
113+
IL_0006: nop
114+
IL_0007: ret
115+
} // end of method DevDiv_545497::.ctor
116+
117+
} // end of class DevDiv_545497

‎tests/src/JIT/Regression/JitBlue/DevDiv_545497/DevDiv_545497.ilproj‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
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+
<ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid>
9+
<OutputType>Exe</OutputType>
10+
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
11+
</PropertyGroup>
12+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup>
13+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "></PropertyGroup>
14+
<PropertyGroup>
15+
<DebugType>None</DebugType>
16+
<Optimize>True</Optimize>
17+
</PropertyGroup>
18+
<ItemGroup>
19+
<Compile Include="$(MSBuildProjectName).il" />
20+
</ItemGroup>
21+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
22+
<PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
23+
</Project>

0 commit comments

Comments
 (0)