From 954ae7fda9ff14fb86a8c616ae56eaba5770d0f9 Mon Sep 17 00:00:00 2001 From: Sejong OH Date: Thu, 10 Mar 2016 23:30:23 -0800 Subject: [PATCH] Fix inconsistent uint64-to-double cast Commit migrated from https://github.com/dotnet/coreclr/commit/38d980d00d5cf5c59975dc3e803c373f8836bead --- src/coreclr/src/jit/codegenxarch.cpp | 4 ++ src/coreclr/src/jit/gentree.cpp | 27 ++-------- src/coreclr/src/jit/utils.cpp | 63 ++++++++++++++++++++++ src/coreclr/src/jit/utils.h | 13 +++++ src/coreclr/src/jit/valuenum.cpp | 4 +- .../Regression/JitBlue/GitHub_3449/GitHub_3449.cs | 42 +++++++++++++++ .../JitBlue/GitHub_3449/GitHub_3449.csproj | 50 +++++++++++++++++ .../JIT/Regression/JitBlue/GitHub_3449/app.config | 27 ++++++++++ 8 files changed, 204 insertions(+), 26 deletions(-) mode change 100644 => 100755 src/coreclr/src/jit/gentree.cpp create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_3449/GitHub_3449.cs create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_3449/GitHub_3449.csproj create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_3449/app.config diff --git a/src/coreclr/src/jit/codegenxarch.cpp b/src/coreclr/src/jit/codegenxarch.cpp index c2781ff..cbbd8c7 100644 --- a/src/coreclr/src/jit/codegenxarch.cpp +++ b/src/coreclr/src/jit/codegenxarch.cpp @@ -7502,6 +7502,10 @@ CodeGen::genIntToFloatCast(GenTreePtr treeNode) // result if sign-bit of srcType is set. if (srcType == TYP_ULONG) { + // The instruction sequence below is less accurate than what clang + // and gcc generate. However, we keep the current sequence for backward compatiblity. + // If we change the instructions below, FloatingPointUtils::convertUInt64ToDobule + // should be also updated for consistent conversion result. assert(dstType == TYP_DOUBLE); assert(!op1->isContained()); diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp old mode 100644 new mode 100755 index d7d2b64..f8a1eeb --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -9954,7 +9954,7 @@ CHK_OVF: case TYP_DOUBLE: if ((tree->gtFlags & GTF_UNSIGNED) && lval1 < 0) { - d1 = (double) (unsigned __int64) lval1; + d1 = FloatingPointUtils::convertUInt64ToDouble((unsigned __int64)lval1); } else { @@ -10074,29 +10074,8 @@ CHK_OVF: lval1 = INT64(d1); goto CNS_LONG; case TYP_ULONG: - if (d1 >= 0.0) - { - // Work around a C++ issue where it doesn't properly convert large positive doubles - const double two63 = 2147483648.0 * 4294967296.0; - if (d1 < two63) { - lval1 = UINT64(d1); - } - else { - // subtract 0x8000000000000000, do the convert then add it back again - lval1 = INT64(d1 - two63) + I64(0x8000000000000000); - } - goto CNS_LONG; - } - - // This double cast to account for an ECMA spec hole. - // When converting from a double to an unsigned the ECMA - // spec states that a conforming implementation should - // "truncate to zero." However that doesn't make much sense - // when the double in question is negative and the target - // is unsigned. gcc converts a negative double to zero when - // cast to an unsigned. To make gcc conform to MSVC behavior - // this cast is necessary. - lval1 = UINT64(INT64(d1)); goto CNS_LONG; + lval1 = FloatingPointUtils::convertDoubleToUInt64(d1); + goto CNS_LONG; case TYP_FLOAT: d1 = forceCastToFloat(d1); diff --git a/src/coreclr/src/jit/utils.cpp b/src/coreclr/src/jit/utils.cpp index e13009f..2c3d20c 100644 --- a/src/coreclr/src/jit/utils.cpp +++ b/src/coreclr/src/jit/utils.cpp @@ -1544,3 +1544,66 @@ unsigned CountDigits(unsigned num, unsigned base /* = 10 */) } #endif // DEBUG + + +double FloatingPointUtils::convertUInt64ToDouble(unsigned __int64 uIntVal) { + __int64 s64 = uIntVal; + double d; + if (s64 < 0) { +#if defined(_TARGET_XARCH_) + // RyuJIT codegen and clang (or gcc) may produce different results for casting uint64 to + // double, and the clang result is more accurate. For example, + // 1) (double)0x84595161401484A0UL --> 43e08b2a2c280290 (RyuJIT codegen or VC++) + // 2) (double)0x84595161401484A0UL --> 43e08b2a2c280291 (clang or gcc) + // If the folding optimization below is implemented by simple casting of (double)uint64_val + // and it is compiled by clang, casting result can be inconsistent, depending on whether + // the folding optimization is triggered or the codegen generates instructions for casting. // + // The current solution is to force the same math as the codegen does, so that casting + // result is always consistent. + + // d = (double)(int64_t)uint64 + 0x1p64 + uint64_t adjHex = 0x43F0000000000000UL; + d = (double)s64 + *(double*)&adjHex; +#else + d = (double)uIntVal; +#endif + } + else + { + d = (double)uIntVal; + } + return d; +} + +float FloatingPointUtils::convertUInt64ToFloat(unsigned __int64 u64) { + double d = convertUInt64ToDouble(u64); + return (float)d; +} + +unsigned __int64 FloatingPointUtils::convertDoubleToUInt64(double d) { + unsigned __int64 u64; + if (d >= 0.0) + { + // Work around a C++ issue where it doesn't properly convert large positive doubles + const double two63 = 2147483648.0 * 4294967296.0; + if (d < two63) { + u64 = UINT64(d); + } + else { + // subtract 0x8000000000000000, do the convert then add it back again + u64 = INT64(d - two63) + I64(0x8000000000000000); + } + return u64; + } + + // This double cast to account for an ECMA spec hole. + // When converting from a double to an unsigned the ECMA + // spec states that a conforming implementation should + // "truncate to zero." However that doesn't make much sense + // when the double in question is negative and the target + // is unsigned. gcc converts a negative double to zero when + // cast to an unsigned. To make gcc conform to MSVC behavior + // this cast is necessary. + u64 = UINT64(INT64(d)); + return u64; +} diff --git a/src/coreclr/src/jit/utils.h b/src/coreclr/src/jit/utils.h index 25ca684..0c66775 100644 --- a/src/coreclr/src/jit/utils.h +++ b/src/coreclr/src/jit/utils.h @@ -510,4 +510,17 @@ struct ListNode } }; +/***************************************************************************** +* Floating point utility class +*/ +class FloatingPointUtils { +public: + + static double convertUInt64ToDouble(unsigned __int64 u64); + + static float convertUInt64ToFloat(unsigned __int64 u64); + + static unsigned __int64 convertDoubleToUInt64(double d); +}; + #endif // _UTILS_H_ diff --git a/src/coreclr/src/jit/valuenum.cpp b/src/coreclr/src/jit/valuenum.cpp index 6378846..16183ac 100644 --- a/src/coreclr/src/jit/valuenum.cpp +++ b/src/coreclr/src/jit/valuenum.cpp @@ -1906,13 +1906,13 @@ ValueNum ValueNumStore::EvalCastForConstantArgs(var_types typ, VNFunc func, Valu case TYP_FLOAT: assert(typ == TYP_FLOAT); if (srcIsUnsigned) - return VNForFloatCon(float(UINT64(arg0Val))); + return VNForFloatCon(FloatingPointUtils::convertUInt64ToFloat(UINT64(arg0Val))); else return VNForFloatCon(float(arg0Val)); case TYP_DOUBLE: assert(typ == TYP_DOUBLE); if (srcIsUnsigned) - return VNForDoubleCon(double(UINT64(arg0Val))); + return VNForDoubleCon(FloatingPointUtils::convertUInt64ToDouble(UINT64(arg0Val))); else return VNForDoubleCon(double(arg0Val)); default: diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_3449/GitHub_3449.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_3449/GitHub_3449.cs new file mode 100644 index 0000000..30c8881 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_3449/GitHub_3449.cs @@ -0,0 +1,42 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information + +using System; + +public class Program +{ + // RyuJIT codegen and clang (or gcc) may produce different results for casting uint64 to + // double, and the clang result is more accurate. For example, + // 1) (double)0x84595161401484A0UL --> 43e08b2a2c280290 (RyuJIT codegen or VC++) + // 2) (double)0x84595161401484A0UL --> 43e08b2a2c280291 (clang or gcc) + // Constant folding in RyuJIT simply does (double)0x84595161401484A0UL in its C++ implementation. + // If it is compiled by clang, the example unsigned value and cast tree node are folded into + // 43e08b2a2c280291, which is different from what the codegen produces. To fix this inconsistency, + // the constant folding is forced to have the same behavior as the codegen, and the result + // must be always 43e08b2a2c280290. + public static int Main(string[] args) + { + //Check if the test is being executed on ARMARCH + bool isProcessorArmArch = false; + string processorArchEnvVar = null; + processorArchEnvVar = Environment.GetEnvironmentVariable("PROCESSOR_ARCHITECTURE"); + + if ((processorArchEnvVar != null) + && (processorArchEnvVar.Equals("ARM", StringComparison.CurrentCultureIgnoreCase) + || processorArchEnvVar.Equals("ARM64", StringComparison.CurrentCultureIgnoreCase))) + { + isProcessorArmArch = true; + } + + ulong u64 = 0x84595161401484A0UL; + double f64 = (double)u64; + long h64 = BitConverter.DoubleToInt64Bits(f64); + long expected_h64 = isProcessorArmArch ? 0x43e08b2a2c280291L : 0x43e08b2a2c280290L; + if (h64 != expected_h64) { + Console.WriteLine(String.Format("Expected: 0x{0:x}\nActual: 0x{1:x}", expected_h64, h64)); + return -1; + } + return 100; + } +} diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_3449/GitHub_3449.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_3449/GitHub_3449.csproj new file mode 100644 index 0000000..8f33be9 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_3449/GitHub_3449.csproj @@ -0,0 +1,50 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + Properties + 512 + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + $(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages + ..\..\ + + 7a9bfb7d + + + + + + + + + False + + + + + True + + + + + + + + + + + + + $(JitPackagesConfigFileDirectory)minimal\project.json + $(JitPackagesConfigFileDirectory)minimal\project.lock.json + + + + + diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_3449/app.config b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_3449/app.config new file mode 100644 index 0000000..62803f5 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_3449/app.config @@ -0,0 +1,27 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file -- 2.7.4