Skip to content

Commit faff920

Browse files
[clr-interp] Fix implicit promotion of I4 to I (#122319)
Ryujit does implicit promotion using zero-extending promotions for some opcode and sign extending for others. I don't see a particularly good rhyme or reason for the current behavior, but it does not match what the interpreter does. This PR adds a test for all the opcodes that experience promotion, and attempts to test all the behaviors correctly. In building it, several issues were found/fixed. This is known to fix the GitHub13501 test. in addition to the various issues found during test development. --------- Co-authored-by: Copilot <[email protected]>
1 parent a69f885 commit faff920

File tree

8 files changed

+1106
-4
lines changed

8 files changed

+1106
-4
lines changed

src/coreclr/interpreter/compiler.cpp

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2505,6 +2505,26 @@ void InterpCompiler::EmitOneArgBranch(InterpOpcode opcode, int32_t ilOffset, int
25052505
}
25062506
}
25072507

2508+
// Determines whether I4 to I8 promotion should use zero-extension (for unsigned operations)
2509+
// or sign-extension (for signed operations), based on the opcode.
2510+
InterpOpcode InterpOpForWideningArgForImplicitUpcast(InterpOpcode opcode)
2511+
{
2512+
switch (opcode)
2513+
{
2514+
case INTOP_BNE_UN_I4:
2515+
case INTOP_BLE_UN_I4:
2516+
case INTOP_BLT_UN_I4:
2517+
case INTOP_BGE_UN_I4:
2518+
case INTOP_BGT_UN_I4:
2519+
case INTOP_ADD_OVF_UN_I4:
2520+
case INTOP_SUB_OVF_UN_I4:
2521+
case INTOP_MUL_OVF_UN_I4:
2522+
return INTOP_CONV_U8_U4;
2523+
default:
2524+
return INTOP_CONV_I8_I4;
2525+
}
2526+
}
2527+
25082528
void InterpCompiler::EmitTwoArgBranch(InterpOpcode opcode, int32_t ilOffset, int insSize)
25092529
{
25102530
CHECK_STACK(2);
@@ -2515,12 +2535,12 @@ void InterpCompiler::EmitTwoArgBranch(InterpOpcode opcode, int32_t ilOffset, int
25152535
// emitting the conditional branch
25162536
if (argType1 == StackTypeI4 && argType2 == StackTypeI8)
25172537
{
2518-
EmitConv(m_pStackPointer - 1, StackTypeI8, INTOP_CONV_I8_I4);
2538+
EmitConv(m_pStackPointer - 1, StackTypeI8, InterpOpForWideningArgForImplicitUpcast(opcode));
25192539
argType1 = StackTypeI8;
25202540
}
25212541
else if (argType1 == StackTypeI8 && argType2 == StackTypeI4)
25222542
{
2523-
EmitConv(m_pStackPointer - 2, StackTypeI8, INTOP_CONV_I8_I4);
2543+
EmitConv(m_pStackPointer - 2, StackTypeI8, InterpOpForWideningArgForImplicitUpcast(opcode));
25242544
}
25252545
else if (argType1 == StackTypeR4 && argType2 == StackTypeR8)
25262546
{
@@ -2756,12 +2776,12 @@ void InterpCompiler::EmitBinaryArithmeticOp(int32_t opBase)
27562776
#if TARGET_64BIT
27572777
if (type1 == StackTypeI8 && type2 == StackTypeI4)
27582778
{
2759-
EmitConv(m_pStackPointer - 1, StackTypeI8, INTOP_CONV_I8_I4);
2779+
EmitConv(m_pStackPointer - 1, StackTypeI8, InterpOpForWideningArgForImplicitUpcast((InterpOpcode)opBase));
27602780
type2 = StackTypeI8;
27612781
}
27622782
else if (type1 == StackTypeI4 && type2 == StackTypeI8)
27632783
{
2764-
EmitConv(m_pStackPointer - 2, StackTypeI8, INTOP_CONV_I8_I4);
2784+
EmitConv(m_pStackPointer - 2, StackTypeI8, InterpOpForWideningArgForImplicitUpcast((InterpOpcode)opBase));
27652785
type1 = StackTypeI8;
27662786
}
27672787
#endif

src/tests/JIT/Methodical/Methodical_d2.csproj

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
<MergedWrapperProjectReference Remove="e*/**/*_d.??proj" />
1313
<ProjectReference Include="../common/localloc_common.ilproj" />
1414
<ProjectReference Include="../common/eh_common.csproj" />
15+
<ProjectReference Include="int64/unsigned/implicit_promotion_il.ilproj" />
1516
</ItemGroup>
1617
<ItemGroup>
1718
<Compile Include="FPtrunc\convr4a.cs" />
@@ -46,6 +47,7 @@
4647
<Compile Include="int64\signed\s_muldiv.cs" />
4748
<Compile Include="int64\superlong\superlong.cs" />
4849
<Compile Include="int64\unsigned\addsub.cs" />
50+
<Compile Include="int64\unsigned\implicit_promotion.cs" />
4951
<Compile Include="int64\unsigned\ldc_mul.cs" />
5052
<Compile Include="int64\unsigned\ldc_mulovf.cs" />
5153
<Compile Include="int64\unsigned\ldfld_mul.cs" />

src/tests/JIT/Methodical/Methodical_do.csproj

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
<ProjectReference Include="cctor/misc/testlib_misc.csproj" />
1212
<ProjectReference Include="Boxing/xlang/sinlib_cs.csproj" Aliases="sinlib_cs" />
1313
<ProjectReference Include="Boxing/xlang/sinlib_il.ilproj" Aliases="sinlib_il" />
14+
<ProjectReference Include="int64/unsigned/implicit_promotion_il.ilproj" />
1415
</ItemGroup>
1516
<ItemGroup>
1617
<Compile Include="Arrays\lcs\lcs.cs" />
@@ -185,6 +186,7 @@
185186
<Compile Include="int64\signed\s_muldiv.cs" />
186187
<Compile Include="int64\superlong\superlong.cs" />
187188
<Compile Include="int64\unsigned\addsub.cs" />
189+
<Compile Include="int64\unsigned\implicit_promotion.cs" />
188190
<Compile Include="int64\unsigned\ldc_mul.cs" />
189191
<Compile Include="int64\unsigned\ldc_mulovf.cs" />
190192
<Compile Include="int64\unsigned\ldfld_mul.cs" />

src/tests/JIT/Methodical/Methodical_r2.csproj

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
<MergedWrapperProjectReference Remove="e*/**/*_r.??proj" />
1313
<ProjectReference Include="../common/localloc_common.ilproj" />
1414
<ProjectReference Include="../common/eh_common.csproj" />
15+
<ProjectReference Include="int64/unsigned/implicit_promotion_il.ilproj" />
1516
</ItemGroup>
1617
<ItemGroup>
1718
<Compile Include="FPtrunc\convr4a.cs" />
@@ -46,6 +47,7 @@
4647
<Compile Include="int64\signed\s_muldiv.cs" />
4748
<Compile Include="int64\superlong\superlong.cs" />
4849
<Compile Include="int64\unsigned\addsub.cs" />
50+
<Compile Include="int64\unsigned\implicit_promotion.cs" />
4951
<Compile Include="int64\unsigned\ldc_mul.cs" />
5052
<Compile Include="int64\unsigned\ldc_mulovf.cs" />
5153
<Compile Include="int64\unsigned\ldfld_mul.cs" />

src/tests/JIT/Methodical/Methodical_ro.csproj

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
<ProjectReference Include="cctor/misc/testlib_misc.csproj" />
1212
<ProjectReference Include="Boxing/xlang/sinlib_cs.csproj" Aliases="sinlib_cs" />
1313
<ProjectReference Include="Boxing/xlang/sinlib_il.ilproj" Aliases="sinlib_il" />
14+
<ProjectReference Include="int64/unsigned/implicit_promotion_il.ilproj" />
1415
</ItemGroup>
1516
<ItemGroup>
1617
<Compile Include="Arrays\lcs\lcs.cs" />
@@ -185,6 +186,7 @@
185186
<Compile Include="int64\signed\s_muldiv.cs" />
186187
<Compile Include="int64\superlong\superlong.cs" />
187188
<Compile Include="int64\unsigned\addsub.cs" />
189+
<Compile Include="int64\unsigned\implicit_promotion.cs" />
188190
<Compile Include="int64\unsigned\ldc_mul.cs" />
189191
<Compile Include="int64\unsigned\ldc_mulovf.cs" />
190192
<Compile Include="int64\unsigned\ldfld_mul.cs" />
Lines changed: 236 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,236 @@
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+
4+
using System;
5+
using Xunit;
6+
using JitTest_implicit_promotion;
7+
using implicit_promotion;
8+
9+
namespace JitTest_implicit_promotion
10+
{
11+
public class Test
12+
{
13+
// These tests attempt to verify that the implicit upcasting from I4 to I which happens on 64 bit platforms
14+
// is done in a consistent manner across all implementations.
15+
// Notable details of interest:
16+
// add.ovf.un, sub.ovf.un, mul.ovf.un upcast without sign-extension.
17+
// div.un, and rem.un upcast with sign-extension.
18+
// clt.un, cgt.un upcast without sign-extension
19+
// bne.un, blt.un, ble.un, bgt.un, bge.un upcast without sign-extension
20+
[Fact]
21+
[ActiveIssue("https://github.com/dotnet/runtime/issues/122398", TestRuntimes.Mono)]
22+
public static void TestUpcastBehavior()
23+
{
24+
unchecked
25+
{
26+
//////////////////////////////////////////////////////////////////
27+
/// Test scenarios where the first operand is I and the second is i32
28+
/////////////////////////////////////////////////////////////////
29+
30+
// add: (int)0x1 + -2
31+
Assert.Equal(Environment.Is64BitProcess ? (nint)(-1) : (nint)(-1), Operator.add_I_i32((nint)0x1, -2));
32+
33+
// add.ovf.un: (int)0x1 + -2
34+
Assert.Equal(Environment.Is64BitProcess ? (nint)(ulong)0x00000000FFFFFFFF : (nint)(-1), Operator.add_ovf_un_I_i32((nint)0x1, -2));
35+
36+
// add.ovf: (int)0x1 + -2
37+
Assert.Equal(Environment.Is64BitProcess ? (nint)(-1) : (nint)(-1), Operator.add_ovf_I_i32((nint)0x1, -2));
38+
39+
// sub: -1 - -1
40+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.sub_I_i32((nint)(-1), -1));
41+
42+
// sub.ovf: -1 - -1
43+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.sub_ovf_I_i32((nint)(-1), -1));
44+
45+
// sub.ovf.un: -1 - -1
46+
Assert.Equal(Environment.Is64BitProcess ? unchecked((nint)(ulong)0xFFFFFFFF00000000) : (nint)0, Operator.sub_ovf_un_I_i32((nint)(-1), -1));
47+
48+
// mul: (int)0x2 * -1
49+
Assert.Equal(Environment.Is64BitProcess ? (nint)(-2) : (nint)(-2), Operator.mul_I_i32((nint)0x2, -1));
50+
51+
// mul.ovf: (int)0x2 * -1
52+
Assert.Equal(Environment.Is64BitProcess ? (nint)(-2) : (nint)(-2), Operator.mul_ovf_I_i32((nint)0x2, -1));
53+
54+
// mul.ovf.un: (int)0x2 * -1
55+
if (!Environment.Is64BitProcess)
56+
{
57+
Assert.Throws<OverflowException>(() => Operator.mul_ovf_un_I_i32((nint)0x2, -1));
58+
}
59+
else
60+
{
61+
Assert.Equal((nint)(ulong)0x1FFFFFFFE, Operator.mul_ovf_un_I_i32((nint)0x2, -1));
62+
}
63+
64+
// div: -1 / -1
65+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.div_I_i32((nint)(-1), -1));
66+
67+
// div.un: -1 / -1
68+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.div_un_I_i32((nint)(-1), -1));
69+
70+
// rem: -1 % -1
71+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.rem_I_i32((nint)(-1), -1));
72+
73+
// rem.un: -1 % -1
74+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.rem_un_I_i32((nint)(-1), -1));
75+
76+
// and: -1 & -1
77+
Assert.Equal(Environment.Is64BitProcess ? (nint)(-1) : (nint)(-1), Operator.and_I_i32((nint)(-1), -1));
78+
79+
// or: 0 | -1
80+
Assert.Equal(Environment.Is64BitProcess ? (nint)(-1) : (nint)(-1), Operator.or_I_i32((nint)0, -1));
81+
82+
// xor: -1 ^ -1
83+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.xor_I_i32((nint)(-1), -1));
84+
85+
// ceq: -1 == -1
86+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.ceq_I_i32((nint)(-1), -1));
87+
88+
// cgt: -2 > -3
89+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.cgt_I_i32((nint)(-2), -3));
90+
91+
// cgt.un: -2 > -1 (unsigned)
92+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.cgt_un_I_i32((nint)(-2), -1));
93+
94+
// clt: -1 < -2
95+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.clt_I_i32((nint)(-1), -2));
96+
97+
// clt.un: -2 < -1 (unsigned)
98+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.clt_un_I_i32((nint)(-2), -1));
99+
100+
// beq: -1 == -1
101+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.beq_I_i32((nint)(-1), -1));
102+
103+
// bne.un: -1 != -1
104+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)0, Operator.bne_un_I_i32((nint)(-1), -1));
105+
106+
// blt: -1 < -1
107+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.blt_I_i32((nint)(-1), -1));
108+
109+
// blt.un: -2 < -1 (unsigned)
110+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)1, Operator.blt_un_I_i32((nint)(-2), -1));
111+
112+
// ble: -1 <= -1
113+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.ble_I_i32((nint)(-1), -1));
114+
115+
// ble.un: -1 <= -1 (unsigned)
116+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)1, Operator.ble_un_I_i32((nint)(-1), -1));
117+
118+
// bgt: -1 > -1
119+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.bgt_I_i32((nint)(-1), -1));
120+
121+
// bgt.un: -1 > -1 (unsigned)
122+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)0, Operator.bgt_un_I_i32((nint)(-1), -1));
123+
124+
// bge: -1 >= -1
125+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.bge_I_i32((nint)(-1), -1));
126+
127+
// bge.un: 0xFFFFFFFF >= -2 (unsigned, special case)
128+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.bge_un_I_i32(unchecked((nint)(nuint)0xFFFFFFFF), -2));
129+
130+
//////////////////////////////////////////////////////////////////
131+
/// Test scenarios where the first operand is i32 and the second is I
132+
/////////////////////////////////////////////////////////////////
133+
134+
// add: -2 + 1
135+
Assert.Equal(Environment.Is64BitProcess ? (nint)(-1) : (nint)(-1), Operator.add_i32_I(-2, (nint)1));
136+
137+
// add.ovf.un: -2 + 1
138+
Assert.Equal(Environment.Is64BitProcess ? (nint)(ulong)0x00000000FFFFFFFF : (nint)(-1), Operator.add_ovf_un_i32_I(-2, (nint)1));
139+
140+
// add.ovf: -2 + 1
141+
Assert.Equal(Environment.Is64BitProcess ? (nint)(-1) : (nint)(-1), Operator.add_ovf_i32_I(-2, (nint)1));
142+
143+
// sub: -1 - -1
144+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.sub_i32_I(-1, (nint)(-1)));
145+
146+
// sub.ovf: -1 - -1
147+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.sub_ovf_i32_I(-1, (nint)(-1)));
148+
149+
// sub.ovf.un: -1 - 1
150+
Assert.Equal(Environment.Is64BitProcess ? (nint)(ulong)0xFFFFFFFE : (nint)(-2), Operator.sub_ovf_un_i32_I(-1, (nint)1));
151+
152+
// mul: -1 * 2
153+
Assert.Equal(Environment.Is64BitProcess ? (nint)(-2) : (nint)(-2), Operator.mul_i32_I(-1, (nint)2));
154+
155+
// mul.ovf: -1 * 2
156+
Assert.Equal(Environment.Is64BitProcess ? (nint)(-2) : (nint)(-2), Operator.mul_ovf_i32_I(-1, (nint)2));
157+
158+
// mul.ovf.un: -1 * 2
159+
if (!Environment.Is64BitProcess)
160+
{
161+
Assert.Throws<OverflowException>(() => Operator.mul_ovf_un_i32_I(-1, (nint)2));
162+
}
163+
else
164+
{
165+
Assert.Equal((nint)(ulong)0x1FFFFFFFE, Operator.mul_ovf_un_i32_I(-1, (nint)2));
166+
}
167+
168+
// div: -1 / -1
169+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.div_i32_I(-1, (nint)(-1)));
170+
171+
// div.un: -1 / -1
172+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.div_un_i32_I(-1, (nint)(-1)));
173+
174+
// rem: -1 % -1
175+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.rem_i32_I(-1, (nint)(-1)));
176+
177+
// rem.un: -1 % -1
178+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.rem_un_i32_I(-1, (nint)(-1)));
179+
180+
// and: -1 & -1
181+
Assert.Equal(Environment.Is64BitProcess ? (nint)(-1) : (nint)(-1), Operator.and_i32_I(-1, (nint)(-1)));
182+
183+
// or: -1 | 0
184+
Assert.Equal(Environment.Is64BitProcess ? (nint)(-1) : (nint)(-1), Operator.or_i32_I(-1, (nint)0));
185+
186+
// xor: -1 ^ -1
187+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.xor_i32_I(-1, (nint)(-1)));
188+
189+
// ceq: -1 == -1
190+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.ceq_i32_I(-1, (nint)(-1)));
191+
192+
// cgt: -2 > -1
193+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.cgt_i32_I(-2, (nint)(-1)));
194+
195+
// cgt.un: -1 > -2 (unsigned)
196+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.cgt_un_i32_I(-1, (nint)(-2)));
197+
198+
// clt: -1 < -2
199+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)0, Operator.clt_i32_I(-1, (nint)(-2)));
200+
201+
// clt.un: -2 < -1 (unsigned)
202+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.clt_un_i32_I(-2, (nint)(-1)));
203+
204+
// beq: -1 == -1
205+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.beq_i32_I(-1, (nint)(-1)));
206+
207+
// bne.un: -1 != -1
208+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)0, Operator.bne_un_i32_I(-1, (nint)(-1)));
209+
210+
// blt: -2 < -1
211+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.blt_i32_I(-2, (nint)(-1)));
212+
213+
// blt.un: -1 < -2 (unsigned)
214+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)0, Operator.blt_un_i32_I(-1, (nint)(-2)));
215+
216+
// ble: -1 <= -1
217+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.ble_i32_I(-1, (nint)(-1)));
218+
219+
// ble.un: -1 <= -2 (unsigned)
220+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)0, Operator.ble_un_i32_I(-1, (nint)(-2)));
221+
222+
// bgt: -1 > -2
223+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.bgt_i32_I(-1, (nint)(-2)));
224+
225+
// bgt.un: -1 > -2 (unsigned)
226+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)1, Operator.bgt_un_i32_I(-1, (nint)(-2)));
227+
228+
// bge: -1 >= -2
229+
Assert.Equal(Environment.Is64BitProcess ? (nint)1 : (nint)1, Operator.bge_i32_I(-1, (nint)(-2)));
230+
231+
// bge.un: -1 >= -2 (unsigned)
232+
Assert.Equal(Environment.Is64BitProcess ? (nint)0 : (nint)1, Operator.bge_un_i32_I(-1, (nint)(-2)));
233+
}
234+
}
235+
}
236+
}

0 commit comments

Comments
 (0)