New API for single-precision math

tannergooding commented 9 years ago

Rationale

The .NET framework does not currently provide scalar single-precision floating-point support for many of the trigonometric, logarithmic, and other common mathematical functions.

Single-precision floating-point support should be provided for these mathematical functions in order to better interop with high-performance, scientific, and multimedia-based applications where single-precision floating-points are the only implementation required and/or used.

Since adding these methods to the existing System.Math class would break backwards compatibility, they should be provided through a separate class that provides the same and/or similar functionality.

Providing these new APIs would:

improve code readability (especially in the non-hardware backed implementations of methods in the System.Numerics.Vector structs)
improve code performance in areas where the single-precision implementation is all that is required
provide better interoperability with high-performance, scientific, and multimedia-based applications where data structures and existing code may be dependent on single-precision implementations of such methods.
provide a more advanced infrastructure for mathematics in the CoreCLR
Proposed API

The proposed API here provides feature-parity with the existing double-precision math functions provided by the framework.

System.BitConverter

public static class BitConverter
{
    public static float Int32BitsToSingle(int);
    public static int SingleToInt32Bits(float);
}

System.Single

public static partial class MathF
{
    public const float E = 2.71828183f;
    public const float PI = 3.14159265f;

    // Trigonometric Functions
    public static float Acos(float);
    public static float Asin(float);
    public static float Atan(float);
    public static float Atan2(float, float);
    public static float Cos(float);
    public static float Sin(float);
    public static float Tan(float);

    // Hyperbolic Functions
    public static float Cosh(float);
    public static float Sinh(float);
    public static float Tanh(float);

    // Exponential Functions
    public static float Exp(float);

    // Logarithmic Functions
    public static float Log(float);
    public static float Log(float, float);
    public static float Log10(float);

    // Power Functions
    public static float Pow(float, float);
    public static float Sqrt(float);

    // Rounding Functions
    public static float Ceiling(float);
    public static float Floor(float);
    public static float Round(float);
    public static float Round(float, int);
    public static float Round(float, int, MidpointRounding);
    public static float Round(float, MidpointRounding);
    public static float Truncate(float);

    // Manipulation Functions
    public static int Sign(float);

    // Minimum, Maximum, and Difference Functions
    public static float Max(float, float);
    public static float Min(float, float);

    // Other Functions
    public static float Abs(float);
    public static float IEEERemainder(float, float);
}

Example Usage

Currently to calculate the Tangent for each member of the System.Numerics.Vector4 struct, you currently have to call the double-precision version of the method and cast to the result to a single-precision value:

public static Vector4 Acos(Vector4 value)
{
    return new Vector4((float)(Math.Acos(value.X)),
                       (float)(Math.Acos(value.Y)),
                       (float)(Math.Acos(value.Z)),
                       (float)(Math.Acos(value.W)));
}

With the proposed changes, this would now be simplified to the following:

public static Vector4 Acos(Vector4 value)
{
    return new Vector4(Mathf.Acos(value.X),
                       Mathf.Acos(value.Y),
                       Mathf.Acos(value.Z),
                       Mathf.Acos(value.W));
}

The System.Numerics library itself is filled with similar examples as are various bits of code in the CoreFX and CoreCLR repositories.

Perf Numbers

All performance tests are implemented as follows:

100,000 iterations are executed
The time of all iterations are aggregated to compute the Total Time
The time of all iterations are averaged to compute the Average Time
A single iteration executes some simple operation, using the function under test, 5000 times

The execution time below is the Total Time for all 100,000 iterations, measured in seconds.

Hardware: Desktop w/ 3.7GHz Quad-Core A10-7850K (AMD) and 16GB RAM

Function	Improvment	Execution Time - Double	Execution Time - Single
Abs	0.199243555%	0.63752649s	0.63625626s
Acos	12.30220910%	11.5265412s	10.1085220s
Asin	18.66801808%	11.9472425s	9.71692911s
Atan	21.10350002%	10.9964683s	8.67582861s
Atan2	20.51327307%	24.3328097s	19.3413540s
Ceiling	12.91487191%	1.87116459s	1.62950608s
Cos	5.026665542%	7.19916547s	6.83728750s
Cosh	16.46166555%	13.5416170s	11.3124413s
Exp	33.67586387%	6.65578424s	4.41439140s
Floor	10.39208688%	1.74655247s	1.56504922s
Log	19.81117664%	6.42244806s	5.15008553s
Log10	18.40605725%	6.75118866s	5.50856101s
Pow	47.85595440%	31.8820155s	16.6245727s
Round	0.976398142%	4.22620632s	4.18494172s
Sin	15.49539339%	5.98022268s	5.05356365s
Sinh	17.96609899%	14.6242270s	11.9968239s
Sqrt	4.676516651%	2.51281945s	2.39530703s
Tan	30.33470555%	9.07290178s	6.32066374s
Tanh	0.108182099%	8.12724112s	8.11844890s

I believe some extra perf will be squeezed out when the intrinsics (such as CORINFO_INTRINSIC_Sqrt) are properly implemented in the VM layer for single-precision values. Without such functionality, it falls back to the double-precision functionality (extra precision, reduced performance) for certain calls.

Pull Request

There is a sample pull request covering these changes available: dotnet/coreclr#5492

Additional Details

This will require several changes in the CoreCLR as well to support the new APIs via FCALLs and Intrinsics.

KrzysztofCwalina commented 9 years ago

Why would we not add float overloads directly to System.Math?

pdelvo commented 9 years ago

This would break compatibility. If you call Math.Sqrt(2f) would now call the single precision overload with less precision instead of implicitly casting it to double.

tannergooding commented 9 years ago

Given the existing API System.Math.Sqrt(double), when you compile the code: double result = System.Math.Sqrt(2) The code compiles as a call to System.Math.Sqrt(2.0D).

However, if you add System.Math.Sqrt(float) and recompile the code: double result = System.Math.Sqrt(2) The code compiles as a call to System.Math.Sqrt(2.0F).

This results in different behavior and provides no warning as there isn't a data loss in the view of the compiler (when there is an actual loss of precision as compared to the previous release).

KrzysztofCwalina commented 9 years ago

Ah, makes sense. Having said that, I wonder if there is some compatible approach that would allow us to have these members on the Math class. Maybe we could add methods with some naming scheme (prefix or suffix) or add a nested type to Math.

tannergooding commented 9 years ago

Well C uses the sqrt(double) and sqrtf(float)... Although the naming convention doesn't exactly fit (seeing as we have Single and not Float).

Single and Fast make sense as prefixes... 32 could be used as a postfix, but is somewhat ambiguous as it could also imply Int32.

I think a nested type would make it more difficult to access/use the methods. So in a language that doesn't allow using static System.Math.SingleMath, you would have to type Math.SingleMath.Sqrt() (or the equivalent) anytime you wanted to call the method.

A couple of other ideas:

Provide the new APIs in the System.Numerics namespace. It would fit well with the new SIMD enabled types and would likely be used in the same scenarios (e.g. gaming, scientific, and multimedia-based applications).
Provide the new APIs as part of the System.Single struct (and equivalent APIs in System.Double, System.Int32, etc). This would match the behavior of the equivalent APIs in the System.Numerics.Vector types.

MikePopoloski commented 9 years ago

Note that Unity already has a Mathf class that does exactly this: https://docs.unity3d.com/ScriptReference/Mathf.html

That's an indicator that such an API is useful and can help influence the vote for a good name.

KrzysztofCwalina commented 9 years ago

@tannergooding why don't you create a formal API proposal? We would discuss it at the next API review. I think, in general, the addition is something we should do. It's just a matter of deciding on the right shape/location/naming of the APIs.

KrzysztofCwalina commented 9 years ago

BTW, you can read about the API review process at https://github.com/dotnet/corefx/wiki/API-Review-Process.

tannergooding commented 9 years ago

@KrzysztofCwalina, I have updated the first comment to more closely resemble a speclet. Please let me know if any additional changes should be made.

tannergooding commented 9 years ago

Updated the speclet to include the full set of proposed API changes and to provided extended details on the changes that would be provided.

AdamsLair commented 9 years ago

While I definitely support the addition of float-specific / single precision Math functions, I think that moving all math functions out of a centralized static class and into each separate type is not a good idea with regards to developer productivity.

Consider the following example code:

var a = 15;
var b = 42;
var c = int.Max(a, b);
// ... A hundred more lines of static int class math ...

"Whoops, I found out I actually need a different type. Let's change this"

var a = 15.0f;
var b = 42.0f;
var c = float.Max(a, b);
// ... A hundred more lines to modify ...

While the example is not a real-world one, you can easily see the problem with this. Having a single static Math class is a big plus, because the compiler is able to find out which method overload to use all by itself with no work for the developer.

I do not have a great solution for the Math backwards compatibility problem, but personally I'd rather continue to write a few characters of casting operators than having to explicitly use type-specific static methods.

ZigMeowNyan commented 9 years ago

I wouldn't mind having int.Max(a,b) styled methods, myself, because, while you wouldn't have the simplicity of a single starting namespace, you would now be able to easily specify the return type and know that return type just by looking at the methods. It might require more developer work, but sometimes it's better to have the option to be more explicit and descriptive.

Also keep in mind that "helper" classes are only helpful if you know about them. Recent arrivals might not be quick to hunt for a Math class. I've had to point it out to several new recruits myself who expected Min/Max methods on the base type. It lends some credence to the practice of putting methods in the base type when all the arguments and the return type match that base type.

That said, I don't necessarily think that we need to deprecate System.Math. I'd just dislike the code duplication of having the same method in two places (or the overhead of passing a call to a method on the base type). I don't seem to recall any Method redirecting functionality in .NET, which would be helpful for situations like this where we'd like a matching static signature in two separate places to point to the same IL. We could probably gear up for a great round of bike-shedding about proper architecture with this.

I'm not personally opposed to the fleshing out of primitives. As an unrelated example, I've seen argument exceptions occur when calling generic extensions that expected to return integers (Int32) as T and instead encountered shorts (Int16). The same with a double as T but receiving a single. Apparently there are certain scenarios where that conversion isn't automatic when the types aren't known at compile time. But there can be a lot of dragons in any given author's generic extension methods.

tannergooding commented 9 years ago

@AdamsLair, thanks for the support.

I agree that moving them out of a centralized static class is probably not the greatest of options. However, unless the CoreCLR/CoreFX team decides they are willing to break backwards compatibility, it seems that we will end up with at least two distinct locations for any additional math APIs.

I think that, logically, it makes sense to provide these methods through the respective primitive types (as @ZigMeowNyan pointed out, many developers assume that they should exist there anyways).

However, I also agree that in terms of refactoring, ease of use, etc.. It would make sense to also provide them through a centralized class (one that can be easily updated in the future without breaking backwards compatibility).

Example:

public struct Double
{
    public static double Sqrt(double x);
}

public static class MathHelper
{
    public static double Sqrt(double x)
    {
        return double.Sqrt(x);
    }

    public static float Sqrt(float x)
    {
        return float.Sqrt(x);
    }
}

public struct Single
{
    public static double Sqrt(double x);
}

or similar to how System.Numerics.Vector does it:

public struct Double
{
    public static double Sqrt(double x);
}

public static class MathHelper<T> where T : struct
{
    [JitIntrinsic]
    public static T Sqrt(T x)
    {
        object value = (object)(x);

        if (typeof(T) == typeof(double))
        {
            value = double.Sqrt((double)(value));
        }
        else if (typeof(T) == typeof(float))
        {
            value = float.Sqrt((float)(value));
        }
        else
        {
            throw new NotSupportedException();
        }

        return (T)(value);
    }
}

public struct Single
{
    public static double Sqrt(double x);
}

I have updated the speclet to include the current open question of: Should the new APIs be provided through the respective primitive types, through a centralized static class, or both?

GeirGrusom commented 9 years ago

Also perhaps something like sincos since it's common to use both.

AdamsLair commented 9 years ago

However, unless the CoreCLR/CoreFX team decides they are willing to break backwards compatibility, it seems that we will end up with at least two distinct locations for any additional math APIs.

Slightly off-topic, but isn't .Net Core the opportunity to break things for once and remove old cruft? As far as I understood, it is a distinct framework similar to .Net, but not interchangeable at all anyway.

Anyway! On Topic:

With regards to backwards compatibility, I think introducing a generic Math class like you proposed might indeed be a very interesting idea.

// Need to come up with a better name
public static class GenericMath<T> where T : struct
{
    public static T Sqrt(T value);
    public static T Abs(T value);
    public static T Min(T first, T second);
    // ...
}

... because this could also be used to provide generic operator access:

    // ...
    public static T Add<U>(T first, U second);
    public static T Subtract<U>(T first, U second);
    public static T Multiply<U>(T first, U second);
    // ...
    public static bool IsGreaterThan<U>(T first, U second);
    // ...

Which is itself an arguable feature, but as far as I'm concerned we don't currently have any way of using operators generically. If I currently want to define a custom Range<T> class, I will have to either write a generic operator implementation myself, or insert giant "if" blocks into each method.

Again, I do not want to propose to add generic operators to the core library - but if they would be to become a thing at some point, having a generic Math class would be a nice synergy and maybe (?) a good spot to add them.

I'm a little troubled at the thought of introducing a generic math class though, mostly due to performance concerns. Is there any way to get this to be as fast as directly using Math? Also, should the class itself be generic, or rather its methods?

cdrnet commented 9 years ago

Slightly off-topic, but isn't .Net Core the opportunity to break things for once and remove old cruft? As far as I understood, it is a distinct framework similar to .Net, but not interchangeable at all anyway.

It is not a distinct framework. Portable libraries on newer PCL profiles are supposed to work seamlessly on both .Net Framework and .Net Core, so they must be fully compatible at least for what is offered by these profiles, which does include System.Math.

tannergooding commented 9 years ago

@AdamsLair I think the even bigger issue than breaking backwards compatibility, is that in doing so, the user would get no warning since System.Int32 to System.Single and System.Single to System.Double are implicit conversions. The compiler would happily recompile the old code and provide precision loss with the consumer (possibly) being completely unaware.

The other issue with implementing a generic Math class or adding generic methods to the existing Math class is that, in existing code, calls such such as Math.Sqrt(1) which resolved to double Math.Sqrt(double), would now resolve as int Math<T>.Sqrt(int) and end up throwing an exception during run time (no error at compile time). Returning an integer in this scenario would also be undesirable.

As for performance, the typeof(T) == typeof(double) code would all get optimized away since only one of the conditional statements will ever return true. This is exactly how System.Numerics.Vector<T> does it (and although it also provides direct intrinsic support), it seems to have little to no issues with performance.

There doesn't seem to be an optimal solution, but it seems to be between three choices:

Break backwards compatibility (if only there was some way to notify users of the API change when recompiling the code, already compiled code shouldn't be an issue unless you are using Reflection - but using Reflection with the Math libraries seems a terrible idea).
Make it hard to refactor (meaning having the new APIs implemented/moved to in the respective primitive type - float Single.Sqrt(float)).
Have a secondary static math class (with a less than optimal name) that will not break backwards compatibility (System.Mathf, System.MathHelp, System.MathHelper, System.Numerics.Math, etc...).

The second option (moving to the respective primitive types) still seems like a great idea, provided that option 1 or 3 is also chosen. However, at least in C#/VB, with only option 2 it would be possible to have code such as:

using static System.Int32;

// ...
var a = 15;
var b = 42;
var c = Max(a, b);    // Resolves to int System.Int32.Max(int, int)
// ...

and refactoring would just be:

using static System.Single;

// ...
var a = 15;
var b = 42;
var c = Max(a, b);    // Resolves to float System.Single.Max(float, float)
// ...

This would of course only be supported in a language where using static is supported. However, you would require refactoring in any case if you didn't use Dim, var, or auto.

tannergooding commented 9 years ago

@GeirGrusom I think SinCos would be a great addition and I've added it and Exp2M1 to the speclet

AdamsLair commented 9 years ago

@tannergooding using static directives in the way you suggest would be not that great for most places, because you usually want to be able to use more than one primitive type for math. And using the shorthand version for exactly one type and the explicit Math.XY or primitive.XY method calls for all else looks a bit ugly to me.

Very nice writeup of the current discussion state though! :+1:

colombod commented 9 years ago

I think that functions as real first class citizen (like CLR level) would make it easier to avoid breaking changes in this scenarios. Unfortunately CoreFX is born "old" due to the backward compatibility required. That being said. I really like your proposal. I think that this kind of extensions should be easier to make for developers. I few video games and rendering based code we had to implement also float 16 algebra to be compatible with types on the GPU. Is the same silly things of WPF being built all around double when the underneath DX layer is then using float :)

tannergooding commented 9 years ago

@AdamsLair Interestingly enough, the following works correctly:

using static System.Double;
using static System.Int32;
using static System.Single;

//...
var x = Max(1, 2);    // Resolves to: int System.Int32.Max(int, int)
var y = Max(1d, 2d);  // Resolves to: double System.Double.Max(double, double)
var z = Max(1f, 2f);  // Resolves to: float System.Single.Max(float, float)
//...

It isn't the most optimal solution, and wouldn't work in languages that don't support using static statements, but it does work.

It's too bad you can't redirect using static statements into a single location, such as:

using static MyMath = System.Double;
using static MyMath = System.Int32;
using static MyMath = System.Single;
// Maybe shorten to: using static MyMath = System.Double, System.Int32, System.Single;

//...
var x = MyMath.Max(1, 2);    // Resolves to: int System.Int32.Max(int, int)
var y = MyMath.Max(1d, 2d);  // Resolves to: double System.Double.Max(double, double)
var z = MyMath.Max(1f, 2f);  // Resolves to: float System.Single.Max(float, float)
//...

Method redirection of some sort (either syntactic sugar, as above, or direct support by the CLI) would be great and could help with a good bit of these issues.

tannergooding commented 9 years ago

@mellinoe, I have provided a pull request to cover these changes (see dotnet/coreclr#710).

Please note that it is still a work-in-progress.

terrajobst commented 9 years ago

Regarding the open question:

Should the new APIs be provided through the respective primitive types, through a centralized static class, or both?

I don't think we'd want to add all these APIs to the core types. In other words, I think having a design that makes the APIs sit on top would be a requirement.

tannergooding commented 8 years ago

Is there any chance this can move forward with an API Design Review? At the very least, I would like to expose single-precision equivalents to the existing double-precision functions (the ones in System.Math).

It would be very helpful (and would allow me to submit a PR against both CoreFX and CoreCLR) if I knew where to expose the new APIs.

The bare minimum that would need to be exposed (so that the single-precision functions exposed are equivalent to the double-precision functions currently exposed) is:

public static class BitConverter
{
    public static float Int32BitsToSingle(int value);
    public static int SingleToInt32Bits(float value) { return default(int); }
}

public static partial class Mathf
{
    public const float PI = 3.14159265f;
    public const float E = 2.71828183f;

    public static float Abs(float x);
    public static float Acos(float x);
    public static float Asin(float x);
    public static float Atan(float x);
    public static float Atan2(float y, float x);
    public static float Ceiling(float x);
    public static float Cos(float x);
    public static float Cosh(float x);
    public static float Exp(float x);
    public static float Floor(float x);
    public static float IEEERemainder(float x, float y);
    public static float Log(float x);
    public static float Log(float x, float y);
    public static float Log10(float x);
    public static float Max(float x, float y);
    public static float Min(float x, float y);
    public static float Pow(float x, float y);
    public static float Round(float x);
    public static float Round(float x, int digits);
    public static float Round(float x, int digits, System.MidpointRounding mode);
    public static float Round(float x, System.MidpointRounding mode);
    public static int Sign(float x);
    public static float Sin(float x);
    public static float Sinh(float x);
    public static float Sqrt(float x);
    public static float Tan(float x);
    public static float Tanh(float x);
    public static float Truncate(float x);
}

SunnyWar commented 8 years ago

I love this proposal.

mellinoe commented 8 years ago

Personally, I like the proposal. I am very familiar with the naming scheme. Unity uses the Mathf name, so I've seen it a million times and already know what it means. I suspect that other folks on the team might have objections to the name since it is only one character different from Math, and we have also shied away from using the "f" suffix elsewhere (we briefly considered it for the Vector2/3/4 types).

cdrnet commented 8 years ago

Note that the term float (where the "f" suffix comes from) does not stand for System.Single in all .Net languages (e.g. in F# float is System.Double while float32 is System.Single).

shmuelie commented 8 years ago

We have done PointF and RectangleF in the framework though

tannergooding commented 8 years ago

Math32 seems like another logicial possibility that would avoid ambiguity for languages (such as F#) where float or f represents the System.Double type.

I think some combination of Math and Single, Scalar, or 32 is the key to having something unambiguous and easily recognizable as to what it does.

tannergooding commented 8 years ago

https://github.com/dotnet/coreclr/pull/5492 -- Probably won't get merged/approved quite yet, but maybe it will help push things along ;)

tannergooding commented 8 years ago

Posting here as well... Some perf numbers from dotnet/coreclr#5492

All performance tests are implemented as follows:

100,000 iterations are executed
The time of all iterations are aggregated to compute the Total Time
The time of all iterations are averaged to compute the Average Time
A single iteration executes some simple operation, using the function under test, 5000 times

All of the functions which were not modified did not show any improvement or regression.

The execution time below is the Total Time for all 100,000 iterations, measured in seconds.

Hardware: Desktop w/ 3.7GHz Quad-Core A10-7850K (AMD) and 16GB RAM

Function	Improvment	Execution Time - Double	Execution Time - Single
Abs	0.199243555%	0.63752649s	0.63625626s
Acos	12.30220910%	11.5265412s	10.1085220s
Asin	18.66801808%	11.9472425s	9.71692911s
Atan	21.10350002%	10.9964683s	8.67582861s
Atan2	20.51327307%	24.3328097s	19.3413540s
Ceiling	12.91487191%	1.87116459s	1.62950608s
Cos	5.026665542%	7.19916547s	6.83728750s
Cosh	16.46166555%	13.5416170s	11.3124413s
Exp	33.67586387%	6.65578424s	4.41439140s
Floor	10.39208688%	1.74655247s	1.56504922s
Log	19.81117664%	6.42244806s	5.15008553s
Log10	18.40605725%	6.75118866s	5.50856101s
Pow	47.85595440%	31.8820155s	16.6245727s
Round	0.976398142%	4.22620632s	4.18494172s
Sin	15.49539339%	5.98022268s	5.05356365s
Sinh	17.96609899%	14.6242270s	11.9968239s
Sqrt	4.676516651%	2.51281945s	2.39530703s
Tan	30.33470555%	9.07290178s	6.32066374s
Tanh	0.108182099%	8.12724112s	8.11844890s

I believe some extra perf will be squeezed out when the intrinsics (such as CORINFO_INTRINSIC_Sqrt) are properly implemented in the VM layer for single-precision values. The PR, as is, does not implement such functionality, so it falls back to the double-precision functionality (extra precision, reduced performance) for certain calls.

Given the nature of this change, and that it requires corresponding changes in CoreFX to update the contract assemblies, I will update the VM layer in a separate PR.

tannergooding commented 8 years ago

I'm working on getting some Linux/Mac numbers and expect to have them sometime tonight.

Daniel-Svensson commented 8 years ago

@tannergooding Great work so far with float Maths :+1:

I would love to know what you think about it as I think that your code in Math32 (except constants) would fit perfectly in System.Numerics.Scalar

tannergooding commented 8 years ago

@Daniel-Svensson, I would love to get this in (in essentially any supported form). The biggest blocker so far is waiting for an more details on getting an API review (as the team is so busy with other things at the moment).

mellinoe commented 8 years ago

@tannergooding , @terrajobst We should try to get this pushed through a design review meeting; I think it's a valuable addition to be made.

tannergooding commented 8 years ago

@mellinoe, just let me know what needs to be done 😄

karelz commented 8 years ago

Please edit the first post with final proposal. If there's agreement, @mellinoe can bring it for API review.

tannergooding commented 8 years ago

@karelz, How specific does the 'final' proposal need to be?

There are, essentially, four separate pieces to the PR:

Single precision functions that should exist for parity with the Double precision functions currently available (Sin, Cos, Tan, Sqrt, etc).
Functions which don't exist for either precision formats, but should likely exist and would have common use (SinCos, FMA, etc)
Functions which don't exist for either precision format, but provide higher precision computations for core scenarios (expm1, log1p, etc)
Functions which don't exist for either precision formats, and would probably have limited use, but would provide feature parity with C/C++ (erf, erfc, tgamma, lgamma, etc)

Would it help to break the APIs out into four separate categories for the proposal?

Additionally, is it sufficient to say that a particular API should be provided, or does it need to have an explicit suggested location (which is where the discussion will likely trend towards, since we can't add to System.Math without breaking back-compat for recompilation)?

mellinoe commented 8 years ago

I think it would make sense to separate out the first bullet from the rest (a la the issue title). Not to say that the latter 3 aren't important; I think we should do those, too. It will be easier to reason about and deliver if we can focus on the first bullet by itself, though.

does it need to have an explicit suggested location

It's something we need to decide, but I think we can proceed with a design review if we have a couple of options to choose from.

I think the part that is ready for review would be this (copy-pasted from a bit above):

Click to expand

``` csharp public static class BitConverter { public static float Int32BitsToSingle(int value); public static int SingleToInt32Bits(float value) { return default(int); } } public static partial class Mathf { public const float PI = 3.14159265f; public const float E = 2.71828183f; public static float Abs(float x); public static float Acos(float x); public static float Asin(float x); public static float Atan(float x); public static float Atan2(float y, float x); public static float Ceiling(float x); public static float Cos(float x); public static float Cosh(float x); public static float Exp(float x); public static float Floor(float x); public static float IEEERemainder(float x, float y); public static float Log(float x); public static float Log(float x, float y); public static float Log10(float x); public static float Max(float x, float y); public static float Min(float x, float y); public static float Pow(float x, float y); public static float Round(float x); public static float Round(float x, int digits); public static float Round(float x, int digits, System.MidpointRounding mode); public static float Round(float x, System.MidpointRounding mode); public static int Sign(float x); public static float Sin(float x); public static float Sinh(float x); public static float Sqrt(float x); public static float Tan(float x); public static float Tanh(float x); public static float Truncate(float x); } ```

As for the "specific-ness" of the proposal: I think we just need to consolidate the discussion above into a regular "proposal". So we'd just need something like

Rationale
Proposed APIs (specific types and members, maybe not necessarily the namespace)
Example usage

All of that is discussed above, just not consolidated.

tannergooding commented 8 years ago

@mellinoe, thanks!

I'll work on updating the post with a 'final' proposal sometime later tonight.

tannergooding commented 8 years ago

@mellinoe, I have updated the original post to be more succinct.

It now has:

Rational
Proposed API
Example Usage
Perf Numbers

The proposed API has been trimmed down to only provide feature-parity for the existing double-precision math functions

The perf numbers are very basic, only cover a single machine configuration, and do not have the VM layer fully implemented (that is intrinsic support would still need to be properly implemented).

Additionally, there is a link to my PR which implements the 'feature-parity' functionality: https://github.com/dotnet/coreclr/pull/5492 (it looks like I have two conflicts which need cleaning up).

tannergooding commented 8 years ago

Please let me know if you feel the original post should be modified further.

shmuelie commented 8 years ago

Just realized but since the ECMA standard says that the CLR VM only has one internal floating point type (I.12.3.1) I'm not sure how portable this idea might be (since on some platforms it might really just be calling the float64 versions for example)

cc: @CarolEidt

tannergooding commented 8 years ago

@SamuelEnglard, Even if the target platform/architecture only has a single floating point format (say 80-bits, as was the case with the x87 FPU), the user still gets the benefit of code that is easier to write/maintain. They just won't get the performance benefit that other platforms/architectures will receive.

This is really very similar to the System.Numerics.Vector types, which will see a performance increase on architectures which actually support SIMD instructions, but will still execute on architectures which don't.

shmuelie commented 8 years ago

@tannergooding I'm not saying don't do, just more that it should be noted (just like how it is with System.Numerics.Vector)

terrajobst commented 8 years ago

The methods themselves look fine and the performance improvements seem to be well worth it. Thanks a ton for that!

We discussed whether we want to introduce a separate type and if so how we should name it. Due to source compat, we can't introduce the methods on Math, but we considered making them static methods on float. We would have to do the same for the existing methods on Math. Ultimately, we decided against it because it felt to be too much of a deviation from the established coding pattern.

We think the best approach is a new type parallel to Math suffixed with an indicator for the type. We should follow the existing patter for Point and Rect where we used a capital F, such as:

public static class MathF

tannergooding commented 8 years ago

Thanks. I will work on updating my PR to match the approved API.

The additional work still required in the PR will be to ensure:

Intrinsic support is properly hooked up
We have a full suite of tests (matching the double-precision coverage).
We have a valid set of performance tests covering the new code

tannergooding commented 8 years ago

Writing the performance tests is dependent on the System.Runtime.Extensions contracts being updated (see dotnet/corefx#12183) ) -- I wrote the existing System.Math performance tests and have equivalent tests for System.MathF written, they just won't compile/run without the contracts update 😄

@jkotas mentioned it might be good to break apart the API additions and the intrinsic support into separate PRs. Please let me know if this is not desired and I will start adding this work to the current PR, otherwise I will log a separate issue to track this work and assign it to myself

I am currently working on porting the existing System.Math tests that are in CoreFX -- these will become part of dotnet/corefx#12183.

There are some unit tests in tests/src/JIT that should be replicated and rewritten to use the System.MathF calls -- I can either do these as part of the current PR, or create a separate work item to track this work

There are various places in CoreFX and CoreCLR (it looks like System.Numerics has the highest concentration) where the code does (float)System.Math... These should probably all be updated to call System.MathF (although this may have a minor impact on precision in some scenarios). This work should probably be done in a separate PR (and will be required in a separate PR for some scenarios).

tannergooding commented 8 years ago

The core change on the CoreCLR side has gone in, so I think we can close this after the CoreFX side goes in.

There are three bugs tracking the finer implementation details still needed (dotnet/coreclr#7689, dotnet/coreclr#7690, and dotnet/coreclr#7691).

karelz commented 8 years ago

Great, thanks! Keep us posted on the progress.

BTW: It seems that you forgot to update the proposal on the top to MathF with capital F. Can you please do the edit to avoid any confusion? Thanks!

dotnet / runtime