disable gtFoldExprConst for conversion from negative double to unsigned long

[hseok-oh]

Related issue: #6667

In ARM architecture, negative double is converted to zero when target is unsigned integer.
Reference comment and implementation: FloatingPointUtils::convertDoubleToUInt64(double d) in jit/utils.cpp

[jkotas]

@dotnet/jit-contrib Would it be better to unconditionally disable gtFoldExprConst in the JIT where it gives undefined or imprecise results instead? We are going to be chasing mismatches between JIT and JIT helper implementations forever otherwise.

[RussKeldorph]

@jkotas' valid question aside, I don't think I would change the behavior of a method called "FastDbl2Lng" so fundamentally without at least changing its name. Alternatively, I haven't gone through all the cases to be certain, but it feels like you ought to be modifying the JIT_Dbl2U* helpers directly and adding #ifdefs to detect the quirkiness of MSVC (#ifdef _MSC_VER?) versus other compilers, which may already not require the range-checking gymnastics currently implemented in the helpers.

[hseok-oh]

I fundamentally agree with @RussKeldorph 's opinion modifying JIT_Dbl2U* helpers directly. But I modified FastDbl2Lng because there was no helper functions in jithelpers.cpp that uses "#ifdefs" in helper functions for different architectures.
If there is no problem, I'll modify helpers directly.

[hseok-oh]

@dotnet-bot test Ubuntu x64 Checked Build and Test please

[hseok-oh]

@dotnet-bot test OSX x64 Checked Build and Test please

[hseok-oh]

@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test please

[hseok-oh]

Modify helper function JIT_Dbl2ULng directly.
Other helpers such as JIT_Dbl2UIntOvf and JIT_Dbl2ULngOvf did not modified, because these function throw exception if the result of conversion is negative.

@jkotas , @RussKeldorph could you please have a look?

[jkotas]

This implementation works fine on Windows everywhere. We should keep using instead of the slower one below.

[hseok-oh]

@jkotas Could you recommend proper flag? We need to keep using upper one on Windows everywhere, and Linux for x86/x64.

[jkotas]

I am still wondering that a better fix would be to remove this #ifdef _TARGET_XARCH_ ifdef in the JIT:

coreclr/src/jit/gentree.cpp

Line 12045 in df50db7

#ifdef _TARGET_XARCH_

, and leave the JIT helper alone.

[RussKeldorph]

(@jkotas I was very confused for a bit since the line you referenced has moved recently. I edited your comment with the stable link, which you can get by pressing 'y' while viewing the unstable link in the browser.)

@hseok-oh FWIW, I agree with @jkotas about gtFoldExprConst. I'm skeptical there is a code quality advantage to folding these cases which are problematic to emulate because the behavior is unspecified in both C++ and IL.

[hseok-oh]

@jkotas, @RussKeldorph I also agree with you about gtFoldExprConst. The most important thing is that we should offer same result always by casting from negative floating point to unsigned integer in CoreCLR.
I think these casting will be not used frequently and optimization gives us limited benefit compared to possibility of undetermined result.

[hseok-oh]

@dotnet-bot test Ubuntu x64 Checked Build and Test please

[hseok-oh]

@jkotas , @RussKeldorph I have modified gtFoldExprConst to disable when convert to unsigned integer from negative double, instead of modifying helper function.

[RussKeldorph]

Should this be if (d1 <= 1.0)? I believe this is consistent with other places that test the same thing.

Also please place opening brace on next line. (Source formatting tool is close...)

[hseok-oh]

@RussKeldorph Is it typo of if (d1 <= -1.0) ?

[hseok-oh]

@jkotas , @RussKeldorph I've modified gtFoldExprConst by using varTypeIsUnsigned.

[hseok-oh]

@jkotas , @RussKeldorph PTAL

[jkotas]

LGTM

[hseok-oh]

@jkotas @RussKeldorph @JosephTremoulet ready to get merged.
Does it have another issues to get merged?

[JosephTremoulet]

LGTM, but probably worth adding a comment indicating that we don't fold these cases because the result is unspecified per ECMA and the native math doing the fold doesn't match the run-time computation on all platforms and we want the behavior to be the same with or without folding.

[hseok-oh]

@jkotas @JosephTremoulet @RussKeldorph PTAL

[JosephTremoulet]

LGTM