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

Update the runtime to have deterministic floating-point to integer conversions #61885

Closed
tannergooding opened this issue Nov 21, 2021 · 19 comments · Fixed by #100993
Closed

Update the runtime to have deterministic floating-point to integer conversions #61885

tannergooding opened this issue Nov 21, 2021 · 19 comments · Fixed by #100993
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@tannergooding
Copy link
Member

tannergooding commented Nov 21, 2021

Issue

.NET currently depends on the underlying platform to perform floating-point to integer conversions. For most scenarios, this behavior is correct and IEEE 754 compliant.

However, for cases of overflow or cases where the underlying platform does not directly support a given conversion, each platform can return different results.

This differing behavior can lead to downstream issues when dealing with such conversions and lead to hard to diagnose bugs.

Proposal

We should follow in the footsteps of other languages and standardize our conversion behavior here to be consistent on all platforms.

In particular, we should standardize our behavior to be consistent with ARM64, Rust, and WASM and to saturate the conversions rather than some other behavior such as using a sentinel value (x86/x64). NaN will likewise be specially handled and overflow to 0.

What's Required

For ARM32 and ARM64, there will be effectively no impact and the codegen will remain identical to what it is today as the underlying conversions already perform saturation on overflow. The exception is for conversions to small integers where there is no direct underlying platform support and so manual clamping of the input to the appropriate range will be required before converting.

For x86 and x64, the platform only directly supports converting to int32 with additional support for converting to int64 on x64. Conversions to small integer types and unsigned types will require the same manual clamping as ARM32/ARM64. Additionally, since the underlying platform returns a sentinel value (0x80000000 or 0x80000000_00000000) we will need to have logic to handle this.

Perf Impact

For the conversions that need additional handling, there is going to be additional cost and measurable perf impact.

For a Skylake processor simple conversions would go from ~7 cycles to approx. between 19 and 24 cycles, or a theoretical "worst case" taking ~3.4x more time.

The measured impact by Rust was much lower and was closer to a 0.8x regression in real world scenarios involving heavy floating-point to integer conversions (in particular an "RBG JPEG encoding" algorithm).

Additional Considerations

We should additionally expose a set of unsafe APIs that perform the "platform specific" conversions. These would allow developers who understand the xplat differences and need the perf to still get the underlying behavior.

It is likely not worth waiting for additional asks on this as other platforms, such as Rust have already exposed such APIs on their end. Additionally, we will likely require them for our own code in the BCL, including for Vector<T>, Vector64<T>, Vector128<T>, and Vector256<T>. This will likewise allow the mentioned vector types to be consistent by default and provide fast fallbacks where appropriate.

I would propose these are exposed as ConvertTo*Unsafe, matching the existing ConvertTo* algorithms we have in several other locations.

This would result in the following additions (the commented out APIs already exist or are approved and will be implemented in .NET 7):

namespace System
{
    public struct Double
    {
        public static int ConvertToInt32(double value);
        public static int ConvertToInt32Unsafe(double value);

        public static long ConvertToInt64(double value);
        public static long ConvertToInt64Unsafe(double value);

        public static uint ConvertToUInt32(double value);
        public static uint ConvertToUInt32Unsafe(double value);

        public static ulong ConvertToUInt64(double value);
        public static ulong ConvertToUInt64Unsafe(double value);
    }

    public struct Half
    {
        public static int ConvertToInt32(Half value);
        public static int ConvertToInt32Unsafe(Half value);

        public static long ConvertToInt64(Half value);
        public static long ConvertToInt64Unsafe(Half value);

        public static uint ConvertToUInt32(Half value);
        public static uint ConvertToUInt32Unsafe(Half value);

        public static ulong ConvertToUInt64(Half value);
        public static ulong ConvertToUInt64Unsafe(Half value);
    }

    public struct Single
    {
        public static int ConvertToInt32(float value);
        public static int ConvertToInt32Unsafe(float value);

        public static long ConvertToInt64(float value);
        public static long ConvertToInt64Unsafe(float value);

        public static uint ConvertToUInt32(float value);
        public static uint ConvertToUInt32Unsafe(float value);

        public static ulong ConvertToUInt64(float value);
        public static ulong ConvertToUInt64Unsafe(float value);
    }
}

namespace System.Numerics
{
    public static class Vector
    {
        // public static Vector<int> ConvertToInt32(Vector<float> value);
        public static Vector<int> ConvertToInt32Unsafe(Vector<float> value);

        // public static Vector<long> ConvertToInt64(Vector<double> value);
        public static Vector<long> ConvertToInt64Unsafe(Vector<double> value);

        // public static Vector<uint> ConvertToUInt32(Vector<float> value);
        public static Vector<uint> ConvertToUInt32Unsafe(Vector<float> value);

        // public static Vector<ulong> ConvertToUInt64(Vector<double> value);
        public static Vector<ulong> ConvertToUInt64Unsafe(Vector<double> value);
    }
}

namespace System.Runtime.Intrinsics
{
    public static class Vector64
    {
        // public static Vector64<int> ConvertToInt32(Vector64<float> value);
        public static Vector64<int> ConvertToInt32Unsafe(Vector64<float> value);

        // public static Vector64<long> ConvertToInt64(Vector64<double> value);
        public static Vector64<long> ConvertToInt64Unsafe(Vector64<double> value);

        // public static Vector64<uint> ConvertToUInt32(Vector64<float> value);
        public static Vector64<uint> ConvertToUInt32Unsafe(Vector64<float> value);

        // public static Vector64<ulong> ConvertToUInt64(Vector64<double> value);
        public static Vector64<ulong> ConvertToUInt64Unsafe(Vector64<double> value);
    }
    
    public static class Vector128
    {
        // public static Vector128<int> ConvertToInt32(Vector128<float> value);
        public static Vector128<int> ConvertToInt32Unsafe(Vector128<float> value);

        // public static Vector128<long> ConvertToInt64(Vector128<double> value);
        public static Vector128<long> ConvertToInt64Unsafe(Vector128<double> value);

        // public static Vector128<uint> ConvertToUInt32(Vector128<float> value);
        public static Vector128<uint> ConvertToUInt32Unsafe(Vector128<float> value);

        // public static Vector128<ulong> ConvertToUInt64(Vector128<double> value);
        public static Vector128<ulong> ConvertToUInt64Unsafe(Vector128<double> value);
    }
    
    public static class Vector256
    {
        // public static Vector256<int> ConvertToInt32(Vector256<float> value);
        public static Vector256<int> ConvertToInt32Unsafe(Vector256<float> value);

        // public static Vector256<long> ConvertToInt64(Vector256<double> value);
        public static Vector256<long> ConvertToInt64Unsafe(Vector256<double> value);

        // public static Vector256<uint> ConvertToUInt32(Vector256<float> value);
        public static Vector256<uint> ConvertToUInt32Unsafe(Vector256<float> value);

        // public static Vector256<ulong> ConvertToUInt64(Vector256<double> value);
        public static Vector256<ulong> ConvertToUInt64Unsafe(Vector256<double> value);
    }
}
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Numerics untriaged New issue has not been triaged by the area owner labels Nov 21, 2021
@ghost
Copy link

ghost commented Nov 21, 2021

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

Issue Details

Issue

.NET currently depends on the underlying platform to perform floating-point to integer conversions. For most scenarios, this behavior is correct and IEEE 754 compliant.

However, for cases of overflow or cases where the underlying platform does not directly support a given conversion, each platform can return different results.

This differing behavior can lead to downstream issues when dealing with such conversions and lead to hard to diagnose bugs.

Proposal

We should follow in the footsteps of other languages and standardize our conversion behavior here to be consistent on all platforms.

In particular, we should standardize our behavior to be consistent with ARM64, Rust, and WASM and to saturate the conversions rather than some other behavior such as using a sentinel value (x86/x64). NaN will likewise be specially handled and overflow to 0.

What's Required

For ARM32 and ARM64, there will be effectively no impact and the codegen will remain identical to what it is today as the underlying conversions already perform saturation on overflow. The exception is for conversions to small integers where there is no direct underlying platform support and so manual clamping of the input to the appropriate range will be required before converting.

For x86 and x64, the platform only directly supports converting to int32 with additional support for converting to int64 on x64. Conversions to small integer types and unsigned types will require the same manual clamping as ARM32/ARM64. Additionally, since the underlying platform returns a sentinel value (0x80000000 or 0x80000000_00000000) we will need to have logic to handle this.

Perf Impact

For the conversions that need additional handling, there is going to be additional cost and measurable perf impact.

For a Skylake processor simple conversions would go from ~7 cycles to approx. between 19 and 24 cycles, or a theoretical "worst case" taking ~3.4x more time.

The measured impact by Rust was much lower and was closer to a 0.8x regression in real world scenarios involving heavy floating-point to integer conversions (in particular an "RBG JPEG encoding" algorithm).

Additional Considerations

We should additionally expose a set of unsafe APIs that perform the "platform specific" conversions. These would allow developers who understand the xplat differences and need the perf to still get the underlying behavior.

It is likely not worth waiting for additional asks on this as other platforms, such as Rust have already exposed such APIs on their end. Additionally, we will likely require them for our own code in the BCL, including for Vector<T>, Vector64<T>, Vector128<T>, and Vector256<T>. This will likewise allow the mentioned vector types to be consistent by default and provide fast fallbacks where appropriate.

I would propose these are exposed as ConvertTo*Unsafe, matching the existing ConvertTo* algorithms we have in several other locations.

This would result in the following additions (the commented out APIs already exist or are approved and will be implemented in .NET 7):

namespace System
{
    public struct Double
    {
        public static int ConvertToInt32(double value);
        public static int ConvertToInt32Unsafe(double value);

        public static long ConvertToInt64(double value);
        public static long ConvertToInt64Unsafe(double value);

        public static uint ConvertToUInt32(double value);
        public static uint ConvertToUInt32Unsafe(double value);

        public static ulong ConvertToUInt64(double value);
        public static ulong ConvertToUInt64Unsafe(double value);
    }

    public struct Half
    {
        public static int ConvertToInt32(Half value);
        public static int ConvertToInt32Unsafe(Half value);

        public static long ConvertToInt64(Half value);
        public static long ConvertToInt64Unsafe(Half value);

        public static uint ConvertToUInt32(Half value);
        public static uint ConvertToUInt32Unsafe(Half value);

        public static ulong ConvertToUInt64(Half value);
        public static ulong ConvertToUInt64Unsafe(Half value);
    }

    public struct Single
    {
        public static int ConvertToInt32(float value);
        public static int ConvertToInt32Unsafe(float value);

        public static long ConvertToInt64(float value);
        public static long ConvertToInt64Unsafe(float value);

        public static uint ConvertToUInt32(float value);
        public static uint ConvertToUInt32Unsafe(float value);

        public static ulong ConvertToUInt64(float value);
        public static ulong ConvertToUInt64Unsafe(float value);
    }
}

namspace System.Numerics
{
    public static class Vector
    {
        // public static Vector<int> ConvertToInt32(Vector<float> value);
        public static Vector<int> ConvertToInt32Unsafe(Vector<float> value);

        // public static Vector<long> ConvertToInt64(Vector<double> value);
        public static Vector<long> ConvertToInt64Unsafe(Vector<double> value);

        // public static Vector<uint> ConvertToUInt32(Vector<float> value);
        public static Vector<uint> ConvertToUInt32Unsafe(Vector<float> value);

        // public static Vector<ulong> ConvertToUInt64(Vector<double> value);
        public static Vector<ulong> ConvertToUInt64Unsafe(Vector<double> value);
    }
}

namespace System.Runtime.Intrinsics
{
    public static class Vector64
    {
        // public static Vector64<int> ConvertToInt32(Vector64<float> value);
        public static Vector64<int> ConvertToInt32Unsafe(Vector64<float> value);

        // public static Vector64<long> ConvertToInt64(Vector64<double> value);
        public static Vector64<long> ConvertToInt64Unsafe(Vector64<double> value);

        // public static Vector64<uint> ConvertToUInt32(Vector64<float> value);
        public static Vector64<uint> ConvertToUInt32Unsafe(Vector64<float> value);

        // public static Vector64<ulong> ConvertToUInt64(Vector64<double> value);
        public static Vector64<ulong> ConvertToUInt64Unsafe(Vector64<double> value);
    }
    
    public static class Vector128
    {
        // public static Vector128<int> ConvertToInt32(Vector128<float> value);
        public static Vector128<int> ConvertToInt32Unsafe(Vector128<float> value);

        // public static Vector128<long> ConvertToInt64(Vector128<double> value);
        public static Vector128<long> ConvertToInt64Unsafe(Vector128<double> value);

        // public static Vector128<uint> ConvertToUInt32(Vector128<float> value);
        public static Vector128<uint> ConvertToUInt32Unsafe(Vector128<float> value);

        // public static Vector128<ulong> ConvertToUInt64(Vector128<double> value);
        public static Vector128<ulong> ConvertToUInt64Unsafe(Vector128<double> value);
    }
    
    public static class Vector256
    {
        // public static Vector256<int> ConvertToInt32(Vector256<float> value);
        public static Vector256<int> ConvertToInt32Unsafe(Vector256<float> value);

        // public static Vector256<long> ConvertToInt64(Vector256<double> value);
        public static Vector256<long> ConvertToInt64Unsafe(Vector256<double> value);

        // public static Vector256<uint> ConvertToUInt32(Vector256<float> value);
        public static Vector256<uint> ConvertToUInt32Unsafe(Vector256<float> value);

        // public static Vector256<ulong> ConvertToUInt64(Vector256<double> value);
        public static Vector256<ulong> ConvertToUInt64Unsafe(Vector256<double> value);
    }
}
Author: tannergooding
Assignees: -
Labels:

area-System.Numerics, untriaged

Milestone: -

@tannergooding
Copy link
Member Author

tannergooding commented Nov 21, 2021

This is being prototyped under #61761.

This had an internal design doc written up by @davidwrighton that largely states the above recommendation, notably differing on the Additional Considerations section.

CC. @jkotas

@hopperpl
Copy link

I would prefer the terminology "fast" over "unsafe".

It is not dangerous, just inaccurate in rare cases. This also fits the flag -ffast-math for gcc, and /fp:fast for msvc better, although it is not exactly the same.

@gfoidl
Copy link
Member

gfoidl commented Nov 21, 2021

We should additionally expose a set of unsafe APIs that perform the "platform specific" conversions.

For me "unsafe" also doesn't seem to be the best choice for the suffix. What about ConvertTo*Native (to reflect the "platform specific" part)?

@tannergooding
Copy link
Member Author

I proposed Unsafe because it is the keyword we've already standardized on for non-deterministic behavior in other areas, such as hardware intrinsics.

Unsafe doesn't have to mean "dangerous", it represents that you lose some level of safety or guarantee. In this case, it means that the result is no longer deterministic across platforms and that you need to be aware of how each platform behaves in this scenario.

@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Nov 23, 2021
@terrajobst terrajobst added the api-approved API was approved in API review, it can be implemented label Nov 23, 2021
@terrajobst
Copy link
Member

terrajobst commented Nov 23, 2021

Video

  • Without generic math/statics in interfaces, we'd prefer a design where those methods live on a dedicated type
  • If we plan to expose them in generic math (and it seems we do), then it seems those need to exist on the primitive types; the only design choice we'd have is to hide them there. This would mean that people using generic math will have one way to call these "unsafe" methods while the people doing regular math/conversions have a different way of calling those.
  • We could also decide that we don't expose these "unsafe" variants not on the generic interfaces and instead tell those developers to use regular type checks and invoke the corresponding methods on the dedicated type. However, for folks doing generic math this might stick out like a sore thumb.
  • It seems we'd prefer a generic patten where we cut down the number of methods by passing in the output type:
    namespace System
    {
        public struct Double
        {
            public static TInteger ConvertToInteger<TInteger>(double value)
                where TInteger: IBinaryInteger;
            public static TInteger ConvertToIntegerNative<TInteger>(double value)
                where TInteger: IBinaryInteger;
        }
    }

@JimMore
Copy link

JimMore commented Nov 23, 2021

Will there be a compiler option to opt-out where this strictness is not required?

@terrajobst
Copy link
Member

@tannergooding is there a design document for the overall changes? We reviewed the necessary APIs, not the proposed changes to code generation and changing the default., which would require more representation from other groups. I should have asked this, but for some reason this didn't occur to me.

@JimMore

Will there be a compiler option to opt-out where this strictness is not required?

That would be an interesting question to ask. In general, I think we try to avoid global modes that change language semantics (like checked/unchecked) but at the same time we often have compatibility switches to get back old behavior. I don't think I've seen enough data to have a fully formed opinion where this would fall.

@JimMore
Copy link

JimMore commented Nov 24, 2021

There is already a C# compiler option for checked/unchecked, so it seems logical (to me, at least) to have a similar option here.

It would be a bit tedious if the only way to get back the performance you previously had, was to find any affected conversions/casts and call ConvertToIntegerNative<TInteger> instead.

@terrajobst
Copy link
Member

There is already a C# compiler option for checked/unchecked, so it seems logical (to me, at least) to have a similar option here.

Right. The believe from the compiler folks is that making checked/unchecked a compiler option was a mistake, so we shouldn't be using it as an example of prior art worth emulating.

@tannergooding
Copy link
Member Author

It would be a bit tedious if the only way to get back the performance you previously had, was to find any affected conversions/casts and call ConvertToIntegerNative instead.

Yes, but that's the same decision that other languages in this space have made.

Having correct and deterministic behavior across platforms is becoming increasingly prevalent and the bugs and issues due to people relying on x86/x64 specific behavior are only becoming more obvious as more libraries move to support things like ARM64.

If it is tedious, its because it needs to be carefully considered on if the differing behavior is actually acceptable and how it may impact a given application.

@JimMore
Copy link

JimMore commented Nov 24, 2021

I agree that having correct and deterministic behavior across platforms is desirable, and I'm glad it's going to be the default.
I'm in no way trying to suggest this is a bad idea, simply that it would be nice to be able to disable it.
There are many applications for which these failure cases do not apply (e.g. will never over/under-flow), yet they will suffer a (not insignificant) drop in performance for the general case.

@tannergooding
Copy link
Member Author

is there a design document for the overall changes? We reviewed the necessary APIs, not the proposed changes to code generation and changing the default., which would require more representation from other groups. I should have asked this, but for some reason this didn't occur to me.

@terrajobst. There was an internal writeup by David Wrighton and otherwise this is the current doc covering everything. I still have to writeup a more extensive breaking change doc that details the specific error boundaries, but implementation wise its covered under the prototype here: #61761

@terrajobst
Copy link
Member

terrajobst commented Nov 24, 2021

Was David's document already reviewed? We should probably have a public version and share it via dotnet/designs. This seems like an impactful change that warrants wider circulation and most likely a breaking change notice for .NET 7.

@tannergooding
Copy link
Member Author

Was David's document already reviewed? We should probably have a public version and share it via dotnet/designs. This seems like an impactful change that warrants wider circulation and most likely a breaking change notice for .NET 7.

It was circulated already yes. And yes, it's on my plate to ensure all things like the design doc, breaking change doc, perf numbers, etc all get done and documented.

@sakno
Copy link
Contributor

sakno commented Nov 30, 2021

I would like to put my 2 cents: the proposal smells like strictfp in Java. It was bad design decision introduced more than 20 years ago. As a result, Math and StrictMath present in the library. There is a jep that proposes to restore strict floating-point semantics and leads to deprecation of strictfp keyword.

Probably, the better choice is to use environment variable or MSBuild property for that.

@tannergooding
Copy link
Member Author

There is a jep that proposes to restore strict floating-point semantics and leads to deprecation of strictfp keyword.

https://openjdk.java.net/jeps/306 forces all platforms to have consistent behavior, which is exactly what this proposal attains.

The problem is that this then leads to real perf issues in certain workloads around graphics, machine learning, or other workloads that frequently deal with floating-point to integer conversions.

This is being handled by the introduction of APIs that specifically cover floating-point to integer conversion using the "native" behavior, which mirrors what other platforms such as Rust have done.

Probably, the better choice is to use environment variable or MSBuild property for that.

This is generally a bad idea for several reasons, including that semantics of your program can no longer be determined simply by reading the code.

Likewise, it would never be fine-grained enough to allow differentiation between methods that depend on consistent behavior vs those that are robust in the face of inconsistent behavior.

@sakno
Copy link
Contributor

sakno commented Dec 1, 2021

Exactly, that's why JEP proposes consistent behavior over all platforms. Otherwise, strictfp causes fragmentation of math API: Math and StrictMath. You trying to introduce the same fragmentation. Developers of business applications often do not recognize the difference between these two classes. I can predict tons of questions on StackOverflow like I'm using math to compute exchange rate of currencies or GPS coordinates. Which API should I use? With or without Unsafe postfix? and of criticism in a manner Java moving away from stricfp, .NET introducing strictfp.

Regardless of the runtime behavior, method naming looks inconsistent across .NET: UnsafeQueueWorkItem, UnsafeOnCompleted use Unsafe as leading word. You proposing to use Unsafe as trailing word. Also, to avoid confusion it's better to place these methods to the separated class: UnsafeArithmetics/NativeArithmetics (hello StrictMath 😄 ) or introduce special operator to C# in symmetry with checked/unchecked conversion (hello strictfp 😄 ) Anyway, I think it's better not to place these methods to frequently used locations in API surface to avoid confusion.

@terrajobst
Copy link
Member

I am concerned about the fragmentation too but I haven't seen the full plan yet, so I don't believe I'm in a position to fully judge this yet. However, doing API design for quite some time now I've come to appreciate that extreme positions are only useful to illustrate a general design philosophy. In practice, the sweet spot is often to give most customers The One True Way of doing things while also having a more advanced API that has wrinkles for the corner cases or tight loops.

Regardless of the runtime behavior, method naming looks inconsistent across .NET: UnsafeQueueWorkItem, UnsafeOnCompleted use Unsafe as leading word. You proposing to use Unsafe as trailing word.

A lot of our naming convention is a result of considering how APIs are grouped in documentation and code completion. Suffixes work better because they collocate both APIs, thus making the developer aware that there is an unsafe (and thus presumably more "lightweight") alternative.

Anyway, I think it's better not to place these methods to frequently used locations in API surface to avoid confusion.

That is a good point and something we have done in other areas. However, to me this depends on how problematic the faster versions will be in practice.

@tannergooding tannergooding modified the milestones: Future, 9.0.0 Aug 11, 2023
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Apr 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants