Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure that constant folding for long->float is handled correctly #90325

Merged
merged 6 commits into from
Aug 14, 2023
Merged

Ensure that constant folding for long->float is handled correctly #90325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8fb6436
Ensure that constant folding for long->float is handled correctly
tannergooding Aug 10, 2023
01ec621
Adding a regression test ensuring long->float conversions are correct…
tannergooding Aug 10, 2023
a366e7a
Print failure info for test
tannergooding Aug 10, 2023
8d33072
Ensure we continue doing the incorrect 2-step conversion for 32-bit, …
tannergooding Aug 11, 2023
a79b11c
Fix build failure
tannergooding Aug 11, 2023
1040717
Ensure test project uses process isolation
tannergooding Aug 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 52 additions & 6 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14881,6 +14881,21 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree)
goto CNS_LONG;

case TYP_FLOAT:
{
#if defined(TARGET_64BIT)
if (tree->IsUnsigned())
{
f1 = (float)UINT32(i1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the problem we were converting UINT32 to double when calling forceCastToFloat, before converting to float in that function?

Seems like we should get rid of forceCastToFloat and forceCastToUInt32 (their comments refer to ancient build compilers). But maybe all casts to/from floats should be explicit function calls to FloatingPointUtils functions. E.g., here maybe add FloatingPointUtils::convertUInt32ToFloat()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the problem was that it was doing uint->double->float which can produce a different result from uint->float in some edge cases.

64-bit currently does uint->long->float; where-as 32-bit currently does uint->long->double->float, so this ensures the constant folding matches that behavior.

Longer term it would be beneficial to use helpers for these conversions, but that is a much bigger and more involved change. I tried to keep this as minimal as possible for .NET 8 to avoid churn/risk.

}
else
{
f1 = (float)INT32(i1);
}
#else
// 32-bit currently does a 2-step conversion, which is incorrect
// but which we are going to take a breaking change around early
// in a release cycle.

if (tree->IsUnsigned())
{
f1 = forceCastToFloat(UINT32(i1));
Expand All @@ -14889,10 +14904,14 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree)
{
f1 = forceCastToFloat(INT32(i1));
}
#endif

d1 = f1;
goto CNS_DOUBLE;
}

case TYP_DOUBLE:
{
if (tree->IsUnsigned())
{
d1 = (double)UINT32(i1);
Expand All @@ -14902,6 +14921,7 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree)
d1 = (double)INT32(i1);
}
goto CNS_DOUBLE;
}

default:
assert(!"Bad CastToType() in gtFoldExprConst() for a cast from int");
Expand Down Expand Up @@ -14982,22 +15002,48 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree)
goto CNS_LONG;

case TYP_FLOAT:
case TYP_DOUBLE:
{
#if defined(TARGET_64BIT)
if (tree->IsUnsigned() && (lval1 < 0))
{
d1 = FloatingPointUtils::convertUInt64ToDouble((unsigned __int64)lval1);
f1 = FloatingPointUtils::convertUInt64ToFloat((unsigned __int64)lval1);
}
else
{
d1 = (double)lval1;
f1 = (float)lval1;
}
#else
// 32-bit currently does a 2-step conversion, which is incorrect
// but which we are going to take a breaking change around early
// in a release cycle.

if (tree->IsUnsigned() && (lval1 < 0))
{
f1 = forceCastToFloat(
FloatingPointUtils::convertUInt64ToDouble((unsigned __int64)lval1));
}
else
{
f1 = forceCastToFloat((double)lval1);
}
#endif

d1 = f1;
goto CNS_DOUBLE;
}

if (tree->CastToType() == TYP_FLOAT)
case TYP_DOUBLE:
{
if (tree->IsUnsigned() && (lval1 < 0))
{
d1 = FloatingPointUtils::convertUInt64ToDouble((unsigned __int64)lval1);
}
else
{
f1 = forceCastToFloat(d1); // truncate precision
d1 = f1;
d1 = (double)lval1;
}
goto CNS_DOUBLE;
}
default:
assert(!"Bad CastToType() in gtFoldExprConst() for a cast from long");
return tree;
Expand Down
35 changes: 35 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Runtime.CompilerServices;
using Xunit;

public class Runtime_90323
{
[MethodImpl(MethodImplOptions.NoInlining)]
private static float ConvertToSingle(long value) => (float)value;

// 32-bit currently performs a 2-step conversion which causes a different result to be produced
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"currently" is a problematic word for a long-lived test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current plan of record is to fix the places that are doing incorrect conversions early in .NET 9.

I can certainly remove "currently" in the interim if that helps?

[ConditionalFact(typeof(TestLibrary.PlatformDetection), nameof(TestLibrary.PlatformDetection.Is64BitProcess))]
public static int TestEntryPoint()
{
bool passed = true;

long value = 0x4000_0040_0000_0001L;

if (ConvertToSingle(value) != (float)(value))
{
Console.WriteLine($"Mismatch between codegen and constant folding: {ConvertToSingle(value)} != {(float)(value)}");
passed = false;
}

if (BitConverter.SingleToUInt32Bits((float)(value)) != 0x5E80_0001) // 4.6116866E+18f
{
Console.WriteLine($"Mismatch between constant folding and expected value: {(float)(value)} != 4.6116866E+18f; 0x{BitConverter.SingleToUInt32Bits((float)(value)):X8} != 0x5E800001");
passed = false;
}

return passed ? 100 : 0;
}
}
12 changes: 12 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Optimize>True</Optimize>
<!-- Needed for CLRTestEnvironmentVariable -->
<RequiresProcessIsolation>true</RequiresProcessIsolation>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
<CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="1" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests using this also require (something like):

<PropertyGroup>
  <!-- Needed for CLRTestEnvironmentVariable -->
  <RequiresProcessIsolation>true</RequiresProcessIsolation>
</PropertyGroup>

<ProjectReference Include="$(TestLibraryProjectPath)" />
</ItemGroup>
</Project>
Loading