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

Commit 64d7b17

Browse files
JosephTremouletRussKeldorph
JosephTremoulet
authored and
RussKeldorph
committed
Unsuppress same-reg zero-extending mov (x64)
Update CodeGen::genIntToIntCast to stop suppressing 32-bit same-register `mov`s, and to stop assuming that 32-bit enregistered sources already have the top half of their register clear. This latter assumption is usually true, but is not guaranteed across function boundaries by the ABI. As it happens, the runtime code that invokes custom attribute constructors can pass garbage in the top half of such parameters; this change adds a testcase that fails on that path without this fix.
1 parent 1609011 commit 64d7b17

File tree

6 files changed

+206
-12
lines changed

6 files changed

+206
-12
lines changed

‎src/jit/codegenxarch.cpp‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7201,13 +7201,6 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
72017201
// We only need to check for a negative value in sourceReg
72027202
inst_RV_IV(INS_cmp, sourceReg, 0, size);
72037203
genJumpToThrowHlpBlk(EJ_jl, SCK_OVERFLOW);
7204-
if (dstType == TYP_ULONG)
7205-
{
7206-
// cast from TYP_INT to TYP_ULONG
7207-
// The upper bits on sourceReg will already be zero by definition (x64)
7208-
srcType = TYP_ULONG;
7209-
size = EA_8BYTE;
7210-
}
72117204
}
72127205
else
72137206
{
@@ -7259,8 +7252,16 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
72597252
}
72607253
}
72617254

7262-
if (targetReg != sourceReg)
7263-
inst_RV_RV(ins, targetReg, sourceReg, srcType, size);
7255+
if (targetReg != sourceReg
7256+
#ifdef _TARGET_AMD64_
7257+
// On amd64, we can hit this path for a same-register
7258+
// 4-byte to 8-byte widening conversion, and need to
7259+
// emit the instruction to set the high bits correctly.
7260+
|| (EA_ATTR(genTypeSize(dstType)) == EA_8BYTE
7261+
&& EA_ATTR(genTypeSize(srcType)) == EA_4BYTE)
7262+
#endif // _TARGET_AMD64_
7263+
)
7264+
inst_RV_RV(ins, targetReg, sourceReg, srcType, size);
72647265
}
72657266
else // non-overflow checking cast
72667267
{
@@ -7327,7 +7328,14 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode)
73277328
else if (ins == INS_mov)
73287329
{
73297330
noway_assert(!needAndAfter);
7330-
if (targetReg != sourceReg)
7331+
if (targetReg != sourceReg
7332+
#ifdef _TARGET_AMD64_
7333+
// On amd64, 'mov' is the opcode used to zero-extend from
7334+
// 4 bytes to 8 bytes.
7335+
|| (EA_ATTR(genTypeSize(dstType)) == EA_8BYTE
7336+
&& EA_ATTR(genTypeSize(srcType)) == EA_4BYTE)
7337+
#endif // _TARGET_AMD64_
7338+
)
73317339
{
73327340
inst_RV_RV(ins, targetReg, sourceReg, srcType, size);
73337341
}

‎src/jit/emitxarch.cpp‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3707,10 +3707,16 @@ void emitter::emitIns_R_R (instruction ins,
37073707
regNumber reg1,
37083708
regNumber reg2)
37093709
{
3710+
emitAttr size = EA_SIZE(attr);
3711+
37103712
/* We don't want to generate any useless mov instructions! */
3713+
#ifdef _TARGET_AMD64_
3714+
// Same-reg 4-byte mov can be useful because it performs a
3715+
// zero-extension to 8 bytes.
3716+
assert(ins != INS_mov || reg1 != reg2 || size == EA_4BYTE);
3717+
#else
37113718
assert(ins != INS_mov || reg1 != reg2);
3712-
3713-
emitAttr size = EA_SIZE(attr);
3719+
#endif // _TARGET_AMD64_
37143720

37153721
assert(size <= EA_32BYTE);
37163722
noway_assert(emitVerifyEncodable(ins, size, reg1, reg2));

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

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
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+
using System.Reflection;
7+
using System.Runtime.CompilerServices;
8+
9+
// Regression test for bug 200492, in which the x64 codegen for
10+
// integer casts would unconditionally suppress same-register
11+
// 'mov's, which is incorrect for 32-bit same-register 'mov's
12+
// that are needed to clear the upper 32 bits of a 64-bit
13+
// register. The top bits aren't guaranteed to be clear across
14+
// function boundaries, and the runtime code that invokes
15+
// custom attribute constructors passes garbage in those bits,
16+
// so this test (like the original code in the bug report)
17+
// uses custom attribute constructor arguments as the sources
18+
// of the casts in question.
19+
internal class Program
20+
{
21+
[AttributeUsage(AttributeTargets.Method)]
22+
class TestDoubleAttribute : System.Attribute
23+
{
24+
public double Field;
25+
26+
[MethodImpl(MethodImplOptions.NoInlining)]
27+
static double PickDouble(double d, int dummy)
28+
{
29+
return d;
30+
}
31+
public TestDoubleAttribute(uint f)
32+
{
33+
// Need to clear any garbage in the top half of f's
34+
// register before converting to double.
35+
this.Field = PickDouble((double)f, 0);
36+
}
37+
}
38+
39+
[MethodImpl(MethodImplOptions.NoInlining)]
40+
[Program.TestDouble(6)]
41+
public static bool DoubleTest()
42+
{
43+
var methodInfo = typeof(Program).GetTypeInfo().GetDeclaredMethod("DoubleTest");
44+
var attribute = methodInfo.GetCustomAttribute<TestDoubleAttribute>();
45+
46+
return (attribute.Field == (double)6u);
47+
}
48+
49+
[AttributeUsage(AttributeTargets.Method)]
50+
class TestUlongAttribute : System.Attribute
51+
{
52+
public ulong Field;
53+
54+
[MethodImpl(MethodImplOptions.NoInlining)]
55+
void Store(ulong l)
56+
{
57+
Field = l;
58+
}
59+
60+
public TestUlongAttribute(int i)
61+
{
62+
checked {
63+
// Need to clear any garbage in the top half
64+
// of i's register before passing to Store.
65+
Store((ulong)i);
66+
}
67+
}
68+
}
69+
70+
[MethodImpl(MethodImplOptions.NoInlining)]
71+
[Program.TestUlong(6)]
72+
public static bool UlongTest()
73+
{
74+
var methodInfo = typeof(Program).GetTypeInfo().GetDeclaredMethod("UlongTest");
75+
var attribute = methodInfo.GetCustomAttribute<TestUlongAttribute>();
76+
77+
return (attribute.Field == (ulong)6);
78+
}
79+
80+
private static int Main()
81+
{
82+
int errors = 0;
83+
84+
if (!Program.DoubleTest()) {
85+
errors += 1;
86+
}
87+
88+
if (!Program.UlongTest()) {
89+
errors += 1;
90+
}
91+
92+
if (errors > 0) {
93+
Console.WriteLine("Fail");
94+
}
95+
else
96+
{
97+
Console.WriteLine("Pass");
98+
}
99+
100+
return 100 + errors;
101+
}
102+
}

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
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>{E2C84853-0100-4C5D-88C0-355F70483779}</ProjectGuid>
10+
<OutputType>Exe</OutputType>
11+
<AppDesignerFolder>Properties</AppDesignerFolder>
12+
<FileAlignment>512</FileAlignment>
13+
<ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
14+
<ReferencePath>$(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages</ReferencePath>
15+
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
16+
17+
<NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp>
18+
</PropertyGroup>
19+
<!-- Default configurations to help VS understand the configurations -->
20+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
21+
</PropertyGroup>
22+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
23+
</PropertyGroup>
24+
<ItemGroup>
25+
<CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
26+
<Visible>False</Visible>
27+
</CodeAnalysisDependentAssemblyPaths>
28+
</ItemGroup>
29+
<PropertyGroup>
30+
<DebugType></DebugType>
31+
<Optimize>True</Optimize>
32+
</PropertyGroup>
33+
<ItemGroup>
34+
<Compile Include="$(MSBuildProjectName).cs" />
35+
</ItemGroup>
36+
<ItemGroup>
37+
<None Include="project.json" />
38+
<None Include="app.config" />
39+
</ItemGroup>
40+
<ItemGroup>
41+
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
42+
</ItemGroup>
43+
<PropertyGroup>
44+
<ProjectJson>$(JitPackagesConfigFileDirectory)extra\project.json</ProjectJson>
45+
<ProjectLockJson>$(JitPackagesConfigFileDirectory)extra\project.lock.json</ProjectLockJson>
46+
</PropertyGroup>
47+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
48+
<PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
49+
</PropertyGroup>
50+
</Project>

‎tests/src/JIT/Regression/JitBlue/DevDiv_200492/app.config‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?xml version="1.0" encoding="utf-8"?>
2+
<configuration>
3+
<runtime>
4+
<assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
5+
<dependentAssembly>
6+
<assemblyIdentity name="System.Runtime" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
7+
<bindingRedirect oldVersion="0.0.0.0-4.0.20.0" newVersion="4.0.20.0" />
8+
</dependentAssembly>
9+
<dependentAssembly>
10+
<assemblyIdentity name="System.Text.Encoding" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
11+
<bindingRedirect oldVersion="0.0.0.0-4.0.10.0" newVersion="4.0.10.0" />
12+
</dependentAssembly>
13+
<dependentAssembly>
14+
<assemblyIdentity name="System.Threading.Tasks" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
15+
<bindingRedirect oldVersion="0.0.0.0-4.0.10.0" newVersion="4.0.10.0" />
16+
</dependentAssembly>
17+
<dependentAssembly>
18+
<assemblyIdentity name="System.IO" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
19+
<bindingRedirect oldVersion="0.0.0.0-4.0.10.0" newVersion="4.0.10.0" />
20+
</dependentAssembly>
21+
<dependentAssembly>
22+
<assemblyIdentity name="System.Reflection" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
23+
<bindingRedirect oldVersion="0.0.0.0-4.0.10.0" newVersion="4.0.10.0" />
24+
</dependentAssembly>
25+
</assemblyBinding>
26+
</runtime>
27+
</configuration>

‎tests/src/JIT/config/extra/project.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
"System.IO": "4.0.11-rc2-23816",
1111
"System.IO.FileSystem": "4.0.0",
1212
"System.Reflection": "4.1.0-rc2-23816",
13+
"System.Reflection.Extensions": "4.0.1-rc2-23816",
1314
"System.Reflection.TypeExtensions": "4.0.0",
1415
"System.Runtime": "4.1.0-rc2-23816",
1516
"System.Runtime.Extensions": "4.0.10",

0 commit comments

Comments
 (0)