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.

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

INumber.MinNumber and INumber.MaxNumber return NaN in Release configuration #98068

Closed
PavielKraskouski opened this issue Feb 6, 2024 · 9 comments · Fixed by #98125
Closed
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@PavielKraskouski
Copy link

PavielKraskouski commented Feb 6, 2024

Description

When running a program in Release configuration, INumber.MinNumber and INumber.MaxNumber return NaN if one of the numbers is not NaN. In Debug configuration these methods work as expected.

Reproduction Steps

  1. Create a new console project, specify .NET 8 as the target platform.
  2. Add the following code:
int numbers = 0;
int NaN = 0;

for (int i = 0; i < 100000; i++)
{
    float minLength = 0;
    float maxLength = 0;

    if (!float.IsNaN(float.MaxNumber(maxLength / minLength, 1)))
    {
        numbers++;
    }
    else
    {
        NaN++;
    }
}

Console.WriteLine($"Numbers: {numbers}");
Console.WriteLine($"NaN: {NaN}");
  1. Run the program.

Expected behavior

The output of the program should be as follows:

Numbers: 100000
NaN: 0

Actual behavior

Here is the result of the program in Release configuration:

Numbers: 9999
NaN: 90001

Regression?

The bug could not be reproduced in .NET 7. The program works as expected in both configurations.

Known Workarounds

You can use INumberBase.MinMagnitudeNumber and INumberBase.MaxMagnitudeNumber if you intend to work with only positive numbers.

Configuration

.NET:
Microsoft .NET SDK 8.0.101 (x64) from Visual Studio (8.1.123.58017)
Microsoft .NET SDK 8.0.200-preview.23624.5 (x64) from Visual Studio (8.2.23.62405)

OS:
Windows 11 Pro 23H2 22631.3085
Windows 11 Pro 22H2 22621.3085

Architecture:
x64
x64

Other information

The bug is not reproduced if you replace the first argument of MaxNumber with one of the following:

if (!float.IsNaN(float.MaxNumber(0f / 0f, 1)))
{
    numbers++;
}
else
{
    NaN++;
}
if (!float.IsNaN(float.MaxNumber(float.NaN, 1)))
{
    numbers++;
}
else
{
    NaN++;
}

If we run the code in SharpLab and Compiler Explorer, we get 100,000 NaN. On my machines I get 9,999 numbers and 90,001 NaN.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 6, 2024
@ghost
Copy link

ghost commented Feb 6, 2024

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

When running a program in Release configuration, INumber.MinNumber and INumber.MaxNumber return NaN if one of the numbers is not NaN. In Debug configuration these methods work as expected.

Reproduction Steps

  1. Create a new console project, specify .NET 8 as the target platform.
  2. Add the following code:
int numbers = 0;
int NaN = 0;

for (int i = 0; i < 100000; i++)
{
    float minLength = 0;
    float maxLength = 0;

    if (!float.IsNaN(float.MaxNumber(maxLength / minLength, 1)))
    {
        numbers++;
    }
    else
    {
        NaN++;
    }
}

Console.WriteLine($"Numbers: {numbers}");
Console.WriteLine($"NaN: {NaN}");
  1. Run the program.

Expected behavior

The output of the program should be as follows:

Numbers: 100000
NaN: 0

Actual behavior

Here is the result of the program in Release configuration:

Numbers: 9999
NaN: 90001

Regression?

The bug could not be reproduced in .NET 7. The program works as expected in both configurations.

Known Workarounds

You can use INumberBase.MinMagnitudeNumber and INumberBase.MaxMagnitudeNumber if you intend to work with only positive numbers.

Configuration

.NET:
Microsoft .NET SDK 8.0.101 (x64) from Visual Studio (8.1.123.58017)
Microsoft .NET SDK 8.0.200-preview.23624.5 (x64) from Visual Studio (8.2.23.62405)

OS:
Windows 11 Pro 23H2 22631.3085
Windows 11 Pro 22H2 22621.3085

Architecture:
x64
x64

Other information

The bug is not reproduced if you replace the first argument of MaxNumber with one of the following:

if (!float.IsNaN(float.MaxNumber(0f / 0f, 1)))
{
    numbers++;
}
else
{
    NaN++;
}
if (!float.IsNaN(float.MaxNumber(float.NaN, 1)))
{
    numbers++;
}
else
{
    NaN++;
}

If we run the code in SharpLab and Compiler Explorer, we get 100,000 NaN. On my machines I get 9999 numbers and 90001 NaN.

Author: PavielKraskouski
Assignees: -
Labels:

area-System.Numerics, untriaged

Milestone: -

@MichalPetryka
Copy link
Contributor

With TieredCompilation=0 this gives 100000 NaNs, looks like this is a bug in optimized code.

@EgorBo
Copy link
Member

EgorBo commented Feb 7, 2024

Looks like a codegen bug - when jit compiles float.MaxNumber(x, 1f) it imports it as MAXSS(1f, x) while it should be MAXSS(x, 1f). Because if the 1st argument of MAXSS is NaN - it returns the 2nd argument (1f). If the 2nd argument is NaN - it returns NaN.

@EgorBo EgorBo self-assigned this Feb 7, 2024
@EgorBo EgorBo added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed untriaged New issue has not been triaged by the area owner labels Feb 7, 2024
@EgorBo EgorBo added this to the 9.0.0 milestone Feb 7, 2024
@ghost
Copy link

ghost commented Feb 7, 2024

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

When running a program in Release configuration, INumber.MinNumber and INumber.MaxNumber return NaN if one of the numbers is not NaN. In Debug configuration these methods work as expected.

Reproduction Steps

  1. Create a new console project, specify .NET 8 as the target platform.
  2. Add the following code:
int numbers = 0;
int NaN = 0;

for (int i = 0; i < 100000; i++)
{
    float minLength = 0;
    float maxLength = 0;

    if (!float.IsNaN(float.MaxNumber(maxLength / minLength, 1)))
    {
        numbers++;
    }
    else
    {
        NaN++;
    }
}

Console.WriteLine($"Numbers: {numbers}");
Console.WriteLine($"NaN: {NaN}");
  1. Run the program.

Expected behavior

The output of the program should be as follows:

Numbers: 100000
NaN: 0

Actual behavior

Here is the result of the program in Release configuration:

Numbers: 9999
NaN: 90001

Regression?

The bug could not be reproduced in .NET 7. The program works as expected in both configurations.

Known Workarounds

You can use INumberBase.MinMagnitudeNumber and INumberBase.MaxMagnitudeNumber if you intend to work with only positive numbers.

Configuration

.NET:
Microsoft .NET SDK 8.0.101 (x64) from Visual Studio (8.1.123.58017)
Microsoft .NET SDK 8.0.200-preview.23624.5 (x64) from Visual Studio (8.2.23.62405)

OS:
Windows 11 Pro 23H2 22631.3085
Windows 11 Pro 22H2 22621.3085

Architecture:
x64
x64

Other information

The bug is not reproduced if you replace the first argument of MaxNumber with one of the following:

if (!float.IsNaN(float.MaxNumber(0f / 0f, 1)))
{
    numbers++;
}
else
{
    NaN++;
}
if (!float.IsNaN(float.MaxNumber(float.NaN, 1)))
{
    numbers++;
}
else
{
    NaN++;
}

If we run the code in SharpLab and Compiler Explorer, we get 100,000 NaN. On my machines I get 9,999 numbers and 90,001 NaN.

Author: PavielKraskouski
Assignees: EgorBo
Labels:

area-System.Numerics, area-CodeGen-coreclr

Milestone: -

@tannergooding
Copy link
Member

We should also validate this works correctly for MinNumber.

We're supposed to have tests validating the configs here, so I wonder if they aren't getting run in the TieredCompilation=0 outerloop?

@tannergooding
Copy link
Member

The issue is this line here: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/importercalls.cpp#L8520

We setup op1 = cnsNode and op2 = otherNode as required for Max and Min, we then need to swap them if isNumber is true, but because we do std::swap(op1, op2) and then inverse the assignments to retNode, we're just assigning back the original values.

Removing that std::swap(op1, op2) will cause the right thing to happen and fix it 🤦

@MichalPetryka
Copy link
Contributor

Worth noting there's a codegen quality regression here too where the isNaN was folded to constant true on .net 7 but isn't anymore since .net 8.

@tannergooding
Copy link
Member

Code quality changes won't meet the backport bar. It's something we can ensure is re-handled for .NET 9.

It likely happened due to using x != x instead of BitConverter, which is ultimately better for the more common non-constant scenarios.

@MichalPetryka
Copy link
Contributor

Code quality changes won't meet the backport bar. It's something we can ensure is re-handled for .NET 9.

It likely happened due to using x != x instead of BitConverter, which is ultimately better for the more common non-constant scenarios.

Yeah, just wanted to note there's further work here after fixing the invalid codegen.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 7, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 8, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants