Ensure that constant folding for long->float is handled correctly #90325
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis resolves #90323
|
tannergooding
force-pushed
the
flt-cns-fold-bug
branch
from
07ffc6f
to
a366e7a
Compare
|
@BruceForstall Please review. CC @dotnet/jit-contrib |
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <Compile Include="$(MSBuildProjectName).cs" /> | ||
| <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="1" /> |
There was a problem hiding this comment.
Tests using this also require (something like):
<PropertyGroup>
<!-- Needed for CLRTestEnvironmentVariable -->
<RequiresProcessIsolation>true</RequiresProcessIsolation>
</PropertyGroup>
| [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 |
There was a problem hiding this comment.
"currently" is a problematic word for a long-lived test.
There was a problem hiding this comment.
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?
| #if defined(TARGET_64BIT) | ||
| if (tree->IsUnsigned()) | ||
| { | ||
| f1 = (float)UINT32(i1); |
There was a problem hiding this comment.
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()?
There was a problem hiding this comment.
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.
It's tracked by #61885, which I've explicitly moved to the 9 milestone |
This resolves #90323 and was raised by an internal partner team.