Skip to content

Commit 4276f06

Browse files
a74nha74nh
authored
Arm64: Support additional condition checks in select nodes (#78223)
Generating a FEQ or FNEU will generate an incorrect compare. With the optimizer code fixed, it's not possible to generate a test to trigger the code in codegen. However, the code as it is was wrong. Given it's untested, it might be better to replace it with some asserts? I've added some general compare consume tests to ensure everything is generated ok.
1 parent 4ff7eca commit 4276f06

File tree

4 files changed

+138
-4
lines changed

4 files changed

+138
-4
lines changed

‎src/coreclr/jit/codegenarm64.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4752,6 +4752,17 @@ void CodeGen::genCodeForSelect(GenTreeConditional* tree)
47524752
const GenConditionDesc& prevDesc = GenConditionDesc::Get(prevCond);
47534753

47544754
emit->emitIns_R_R_R_COND(INS_csel, attr, targetReg, srcReg1, srcReg2, JumpKindToInsCond(prevDesc.jumpKind1));
4755+
4756+
// Some conditions require an additional condition check.
4757+
if (prevDesc.oper == GT_OR)
4758+
{
4759+
emit->emitIns_R_R_R_COND(INS_csel, attr, targetReg, srcReg1, targetReg, JumpKindToInsCond(prevDesc.jumpKind2));
4760+
}
4761+
else if (prevDesc.oper == GT_AND)
4762+
{
4763+
emit->emitIns_R_R_R_COND(INS_csel, attr, targetReg, targetReg, srcReg2, JumpKindToInsCond(prevDesc.jumpKind2));
4764+
}
4765+
47554766
regSet.verifyRegUsed(targetReg);
47564767
genProduceReg(tree);
47574768
}

‎src/coreclr/jit/optimizer.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4853,7 +4853,8 @@ bool Compiler::optIfConvert(BasicBlock* block)
48534853
}
48544854

48554855
// Invert the condition.
4856-
cond->gtOper = GenTree::ReverseRelop(cond->gtOper);
4856+
GenTree* revCond = gtReverseCond(cond);
4857+
assert(cond == revCond); // Ensure `gtReverseCond` did not create a new node.
48574858

48584859
// Create a select node.
48594860
GenTreeConditional* select =

‎src/tests/JIT/opt/Compares/compares.cs

Lines changed: 118 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,111 @@ public class SideEffects
7272

7373
[MethodImpl(MethodImplOptions.NoInlining)]
7474
public static bool EqualsOrGreaterThan_MaxValue_LHSConst_SideEffects(SideEffects c) => c.B <= 255;
75+
76+
77+
[MethodImpl(MethodImplOptions.NoInlining)]
78+
public static void consume<T>(T a1, T a2) {}
79+
80+
/* If conditions that are consumed. */
81+
82+
[MethodImpl(MethodImplOptions.NoInlining)]
83+
public static void Eq_byte_consume(byte a1, byte a2) {
84+
//ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}}
85+
//ARM64-FULL-LINE-NEXT: csel {{w[0-9]+}}, {{w[0-9]+}}, {{w[0-9]+}}, eq
86+
if (a1 == a2) { a1 = 10; }
87+
consume<byte>(a1, a2);
88+
}
89+
90+
[MethodImpl(MethodImplOptions.NoInlining)]
91+
public static void Ne_short_consume(short a1, short a2)
92+
{
93+
//ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}}
94+
//ARM64-NEXT-FULL-LINE: csel {{w[0-9]+}}, {{w[0-9]+}}, {{w[0-9]+}}, ne
95+
if (a1 != a2) { a1 = 11; }
96+
consume<short>(a1, a2);
97+
}
98+
99+
[MethodImpl(MethodImplOptions.NoInlining)]
100+
public static void Lt_int_consume(int a1, int a2)
101+
{
102+
//ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}}
103+
//ARM64-NEXT-FULL-LINE: csel {{w[0-9]+}}, {{w[0-9]+}}, {{w[0-9]+}}, lt
104+
if (a1 < a2) { a1 = 12; }
105+
consume<int>(a1, a2);
106+
}
107+
108+
[MethodImpl(MethodImplOptions.NoInlining)]
109+
public static void Le_long_consume(long a1, long a2)
110+
{
111+
//ARM64-FULL-LINE: cmp {{x[0-9]+}}, {{x[0-9]+}}
112+
//ARM64-NEXT-FULL-LINE: csel {{x[0-9]+}}, {{x[0-9]+}}, {{x[0-9]+}}, le
113+
if (a1 <= a2) { a1 = 13; }
114+
consume<long>(a1, a2);
115+
}
116+
117+
[MethodImpl(MethodImplOptions.NoInlining)]
118+
public static void Gt_ushort_consume(ushort a1, ushort a2)
119+
{
120+
//ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}}
121+
//ARM64-NEXT-FULL-LINE: csel {{w[0-9]+}}, {{w[0-9]+}}, {{w[0-9]+}}, gt
122+
if (a1 > a2) { a1 = 14; }
123+
consume<ushort>(a1, a2);
124+
}
125+
126+
[MethodImpl(MethodImplOptions.NoInlining)]
127+
public static void Ge_uint_consume(uint a1, uint a2)
128+
{
129+
//ARM64-FULL-LINE: cmp {{w[0-9]+}}, {{w[0-9]+}}
130+
//ARM64-NEXT-FULL-LINE: csel {{w[0-9]+}}, {{w[0-9]+}}, {{w[0-9]+}}, ge
131+
if (a1 >= a2) { a1 = 15; }
132+
consume<uint>(a1, a2);
133+
}
134+
135+
[MethodImpl(MethodImplOptions.NoInlining)]
136+
public static void Eq_ulong_consume(ulong a1, ulong a2)
137+
{
138+
//ARM64-FULL-LINE: cmp {{x[0-9]+}}, {{x[0-9]+}}
139+
//ARM64-NEXT-FULL-LINE: csel {{x[0-9]+}}, {{x[0-9]+}}, {{x[0-9]+}}, eq
140+
if (a1 == a2) { a1 = 16; }
141+
consume<ulong>(a1, a2);
142+
}
143+
144+
[MethodImpl(MethodImplOptions.NoInlining)]
145+
public static void Ne_float_int_consume(float f1, float f2, int a1, int a2)
146+
{
147+
//ARM64-FULL-LINE: fcmp {{s[0-9]+}}, {{s[0-9]+}}
148+
//ARM64-NEXT-FULL-LINE: csel {{w[0-9]+}}, {{w[0-9]+}}, {{w[0-9]+}}, ne
149+
if (f1 != f2) { a1 = 17; }
150+
consume<float>(a1, a2);
151+
}
152+
153+
[MethodImpl(MethodImplOptions.NoInlining)]
154+
public static void Lt_double_long_consume(double f1, double f2, long a1, long a2)
155+
{
156+
//ARM64-FULL-LINE: fcmp {{d[0-9]+}}, {{d[0-9]+}}
157+
//ARM64-NEXT-FULL-LINE: csel {{x[0-31]}}, {{x[0-31]}}, {{x[0-31]}}, lt
158+
if (f1 < f2) { a1 = 18; }
159+
consume<double>(a1, a2);
160+
}
161+
162+
[MethodImpl(MethodImplOptions.NoInlining)]
163+
public static void Eq_double_long_consume(double f1, double f2, long a1, long a2)
164+
{
165+
//ARM64-FULL-LINE: fcmp {{d[0-9]+}}, {{d[0-9]+}}
166+
//ARM64-NEXT-FULL-LINE: csel {{x[0-31]}}, {{x[0-31]}}, {{x[0-31]}}, eq
167+
if (f1 == f2) { a1 = 18; }
168+
consume<double>(a1, a2);
169+
}
170+
171+
[MethodImpl(MethodImplOptions.NoInlining)]
172+
public static void Ne_double_int_consume(double f1, double f2, int a1, int a2)
173+
{
174+
//ARM64-FULL-LINE: fcmp {{d[0-9]+}}, {{d[0-9]+}}
175+
//ARM64-NEXT-FULL-LINE: csel {{w[0-9]+}}, {{w[0-9]+}}, {{w[0-9]+}}, ne
176+
if (f1 != f2) { a1 = 18; }
177+
consume<double>(a1, a2);
178+
}
179+
75180
public static int Main()
76181
{
77182
// Optimize comparison with full range values
@@ -197,7 +302,19 @@ public static int Main()
197302
Console.WriteLine("FullRangeComparisonTest:EqualsOrGreaterThan_MaxValue_LHSConst_SideEffects(null) failed");
198303
return 101;
199304
}
200-
305+
306+
Eq_byte_consume(10, 11);
307+
Ne_short_consume(10, 11);
308+
Lt_int_consume(10, 11);
309+
Le_long_consume(10, 11);
310+
Gt_ushort_consume(10, 11);
311+
Ge_uint_consume(10, 11);
312+
Eq_ulong_consume(10, 11);
313+
Ne_float_int_consume(10.1F, 11.1F, 12, 13);
314+
Lt_double_long_consume(10.1, 11.1, 12, 13);
315+
Eq_double_long_consume(10.1, 11.1, 12, 13);
316+
Ne_double_int_consume(10.1, 11.1, 12, 13);
317+
201318
Console.WriteLine("PASSED");
202319
return 100;
203320
}

‎src/tests/JIT/opt/Compares/compares.csproj

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,15 @@
33
<OutputType>Exe</OutputType>
44
</PropertyGroup>
55
<PropertyGroup>
6-
<DebugType>PdbOnly</DebugType>
6+
<DebugType>None</DebugType>
77
<Optimize>True</Optimize>
88
</PropertyGroup>
99
<ItemGroup>
10-
<Compile Include="$(MSBuildProjectName).cs" />
10+
<Compile Include="$(MSBuildProjectName).cs">
11+
<HasDisasmCheck>true</HasDisasmCheck>
12+
</Compile>
13+
14+
<CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
15+
<CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />
1116
</ItemGroup>
1217
</Project>

0 commit comments

Comments
 (0)