From 8fb6436d7e65b60642cf91c2dcaab52f74dbc996 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 10 Aug 2023 09:43:29 -0700 Subject: [PATCH 1/6] Ensure that constant folding for long->float is handled correctly --- src/coreclr/jit/gentree.cpp | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index c7df58af664bdd..0bc82b7de4ab15 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -14881,18 +14881,22 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) goto CNS_LONG; case TYP_FLOAT: + { if (tree->IsUnsigned()) { - f1 = forceCastToFloat(UINT32(i1)); + f1 = (float)UINT32(i1); } else { - f1 = forceCastToFloat(INT32(i1)); + f1 = (float)INT32(i1); } + d1 = f1; goto CNS_DOUBLE; + } case TYP_DOUBLE: + { if (tree->IsUnsigned()) { d1 = (double)UINT32(i1); @@ -14902,6 +14906,7 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) d1 = (double)INT32(i1); } goto CNS_DOUBLE; + } default: assert(!"Bad CastToType() in gtFoldExprConst() for a cast from int"); @@ -14982,22 +14987,36 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) goto CNS_LONG; case TYP_FLOAT: - case TYP_DOUBLE: + { + // `long -> double -> float` can produce different results from + // `long -> float`, such as for 0x4000_0040_0000_0001L. So ensure + // we handle this directly. + if (tree->IsUnsigned() && (lval1 < 0)) { - d1 = FloatingPointUtils::convertUInt64ToDouble((unsigned __int64)lval1); + f1 = FloatingPointUtils::convertUInt64ToFloat((unsigned __int64)lval1); } else { - d1 = (double)lval1; + f1 = (float)lval1; } - if (tree->CastToType() == TYP_FLOAT) + d1 = f1; + goto CNS_DOUBLE; + } + + case TYP_DOUBLE: + { + if (tree->IsUnsigned() && (lval1 < 0)) { - f1 = forceCastToFloat(d1); // truncate precision - d1 = f1; + d1 = FloatingPointUtils::convertUInt64ToDouble((unsigned __int64)lval1); + } + else + { + d1 = (double)lval1; } goto CNS_DOUBLE; + } default: assert(!"Bad CastToType() in gtFoldExprConst() for a cast from long"); return tree; From 01ec62148312b5f94984d3ab8388014fcb583558 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 10 Aug 2023 09:49:10 -0700 Subject: [PATCH 2/6] Adding a regression test ensuring long->float conversions are correctly handled --- .../JitBlue/Runtime_90323/Runtime_90323.cs | 29 +++++++++++++++++++ .../Runtime_90323/Runtime_90323.csproj | 9 ++++++ 2 files changed, 38 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs b/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs new file mode 100644 index 00000000000000..3f419c89e4b1cf --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs @@ -0,0 +1,29 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; +using Xunit; + +public class Runtime_90323 +{ + [MethodImpl(MethodImplOptions.NoInlining)] + private static float ConvertToSingle(long value) => (float)value; + + [Fact] + public static int TestEntryPoint() + { + long value = 0x4000_0040_0000_0001L; + + if (ConvertToSingle(value) != (float)(value)) + { + return 0; + } + + if ((float)(value) != 4.6116866E+18f) + { + return 0; + } + + return 100; + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.csproj new file mode 100644 index 00000000000000..36cb572b261543 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.csproj @@ -0,0 +1,9 @@ + + + True + + + + + + From a366e7aecb8e2c4aea2e074f31e818b5bf2d691a Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 10 Aug 2023 13:15:33 -0700 Subject: [PATCH 3/6] Print failure info for test --- .../JitBlue/Runtime_90323/Runtime_90323.cs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs b/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs index 3f419c89e4b1cf..278e3097abbf1a 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs @@ -1,6 +1,7 @@ // 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; @@ -12,18 +13,22 @@ public class Runtime_90323 [Fact] public static int TestEntryPoint() { + bool passed = true; + long value = 0x4000_0040_0000_0001L; if (ConvertToSingle(value) != (float)(value)) { - return 0; + Console.WriteLine($"Mismatch between codegen and constant folding: {ConvertToSingle(value)} != {(float)(value)}"); + passed = false; } - if ((float)(value) != 4.6116866E+18f) + if (BitConverter.SingleToUInt32Bits((float)(value)) != 0x5E80_0001) // 4.6116866E+18f { - return 0; + Console.WriteLine($"Mismatch between constant folding and expected value: {(float)(value)} != 4.6116866E+18f; {BitConverter.SingleToUInt32Bits((float)(value))} != 0x5E80_0001"); + passed = false; } - return 100; + return passed ? 100 : 0; } } From 8d33072d8058002c5d96156eca88440246ce09d3 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Fri, 11 Aug 2023 10:13:25 -0700 Subject: [PATCH 4/6] Ensure we continue doing the incorrect 2-step conversion for 32-bit, to match codegen --- src/coreclr/jit/gentree.cpp | 34 ++++++++++++++++--- .../JitBlue/Runtime_90323/Runtime_90323.cs | 5 +-- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 0bc82b7de4ab15..cb3b6d264c8643 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -14882,6 +14882,7 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) case TYP_FLOAT: { +#if defined(TARGET_64BIT) if (tree->IsUnsigned()) { f1 = (float)UINT32(i1); @@ -14890,6 +14891,20 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) { 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)); + } + else + { + f1 = forceCastToFloat(INT32(i1)); + } +#endif d1 = f1; goto CNS_DOUBLE; @@ -14988,10 +15003,7 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) case TYP_FLOAT: { - // `long -> double -> float` can produce different results from - // `long -> float`, such as for 0x4000_0040_0000_0001L. So ensure - // we handle this directly. - +#if defined(TARGET_64BIT) if (tree->IsUnsigned() && (lval1 < 0)) { f1 = FloatingPointUtils::convertUInt64ToFloat((unsigned __int64)lval1); @@ -15000,6 +15012,20 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) { 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; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs b/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs index 278e3097abbf1a..0be54b47cd659e 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs @@ -10,7 +10,8 @@ public class Runtime_90323 [MethodImpl(MethodImplOptions.NoInlining)] private static float ConvertToSingle(long value) => (float)value; - [Fact] + // 32-bit currently performs a 2-step conversion which causes a different result to be produced + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))] public static int TestEntryPoint() { bool passed = true; @@ -25,7 +26,7 @@ public static int TestEntryPoint() if (BitConverter.SingleToUInt32Bits((float)(value)) != 0x5E80_0001) // 4.6116866E+18f { - Console.WriteLine($"Mismatch between constant folding and expected value: {(float)(value)} != 4.6116866E+18f; {BitConverter.SingleToUInt32Bits((float)(value))} != 0x5E80_0001"); + Console.WriteLine($"Mismatch between constant folding and expected value: {(float)(value)} != 4.6116866E+18f; 0x{BitConverter.SingleToUInt32Bits((float)(value)):X8} != 0x5E800001"); passed = false; } From a79b11c0bf2dba99c079cb64c0938bb52ea7bcf2 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Fri, 11 Aug 2023 11:16:40 -0700 Subject: [PATCH 5/6] Fix build failure --- src/coreclr/jit/gentree.cpp | 3 ++- .../JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index cb3b6d264c8643..d02ce4714d8a2e 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -15019,7 +15019,8 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) if (tree->IsUnsigned() && (lval1 < 0)) { - f1 = forceCastToFloat(FloatingPointUtils::convertUInt64ToDouble((unsigned __int64)lval1)); + f1 = forceCastToFloat( + FloatingPointUtils::convertUInt64ToDouble((unsigned __int64)lval1)); } else { diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs b/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs index 0be54b47cd659e..2e6c5a50c3e6ef 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.cs @@ -11,7 +11,7 @@ public class Runtime_90323 private static float ConvertToSingle(long value) => (float)value; // 32-bit currently performs a 2-step conversion which causes a different result to be produced - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))] + [ConditionalFact(typeof(TestLibrary.PlatformDetection), nameof(TestLibrary.PlatformDetection.Is64BitProcess))] public static int TestEntryPoint() { bool passed = true; From 1040717bd369dcd4995b5ae4a0eac2fc76e672ea Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Fri, 11 Aug 2023 12:47:44 -0700 Subject: [PATCH 6/6] Ensure test project uses process isolation --- .../JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.csproj | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.csproj index 36cb572b261543..6175d4edefd0a5 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.csproj +++ b/src/tests/JIT/Regression/JitBlue/Runtime_90323/Runtime_90323.csproj @@ -1,9 +1,12 @@ True + + true +