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

[System.MathF] Bring support for the single-precission Math operations to Mono #7941

Merged
merged 8 commits into from Apr 5, 2018

Conversation

Projects
None yet
8 participants
@migueldeicaza
Member

migueldeicaza commented Apr 2, 2018

This pull request brings support to Mono's class libraries for the new System.MathF type which includes operations for single-floating point.

This is needed to run some new applications like:

https://github.com/aras-p/ToyPathTracer

With this change, it is possible to run this unmodified. With this support, and the -O=float32 option, it is possible to improve the performance from 6.3Mrays/sec to 7.8Mrays/sec on a Mac.

Oddly, this seems to be faster than .NET Core 2.1.4, which is at 4.2Mrays/sec.

#if MONO
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static byte Clamp(byte value, byte min, byte max)
{

This comment has been minimized.

@vargaz

vargaz Apr 2, 2018

Member

This should use K&R indentation.

This comment has been minimized.

@migueldeicaza

migueldeicaza Apr 2, 2018

Member

I copy pasted this from CoreCLR, I rather not change the style of the copy-pasted code.

This comment has been minimized.

@marek-safar

marek-safar Apr 4, 2018

Member

I am tentative on this, would much rather see directly used ../external/corefx/src/Common/src/CoreLib/System/Math.cs but we could do it as follow up change

@@ -316,3 +316,142 @@ ves_icall_System_Math_SplitFractionDouble (gdouble *v)
{
return modf (*v, v);
}
gfloat
ves_icall_System_MathF_Acos (gfloat x)

This comment has been minimized.

@vargaz

vargaz Apr 2, 2018

Member

This should be just float, we don't use gfloat.

This comment has been minimized.

@migueldeicaza

migueldeicaza Apr 2, 2018

Member

We do in that file, just a couple of lines above ;-)

I dont mind updating.

@@ -439,6 +439,31 @@ ICALL(MATH_16, "Sqrt", ves_icall_System_Math_Sqrt)
ICALL(MATH_17, "Tan", ves_icall_System_Math_Tan)
ICALL(MATH_18, "Tanh", ves_icall_System_Math_Tanh)
ICALL_TYPE(MATHF, "System.MathF", MATHF_1)
ICALL(MATHF_1, "Acos", ves_icall_System_MathF_Acos)

This comment has been minimized.

@vargaz

vargaz Apr 2, 2018

Member

These can be HANDLES(..) I think.

This comment has been minimized.

@migueldeicaza

migueldeicaza Apr 2, 2018

Member

What do you mean?

This comment has been minimized.

@jaykrell

jaykrell Apr 2, 2018

Member

Many but not all lines icall-def.h have HANDLES() around them.
This is how icalls get converted to be coop-friendly.

  1. passes/returns coop handles instead of raw pointers
  2. passes an extra, initialized MonoError*
  3. Check the error at the end.
    Eventually every line should either have HANDLES or some comment or other macro explaining why not -- for example, maybe the only one, if no pointers and no errors and performance sensitive, that combination does not want HANDLES.

This comment has been minimized.

@kumpera

kumpera Apr 2, 2018

Member

@vargaz wouldn't HANDLE'izing those functions produce a net loss of perf for something they don't need? IE, none raise exceptions or take ref pointers.

@vargaz

vargaz approved these changes Apr 2, 2018

ICALL(MATHF_13, "Floor", ves_icall_System_MathF_Floor)
ICALL(MATHF_14, "Log", ves_icall_System_MathF_Log)
ICALL(MATHF_15, "Log10", ves_icall_System_MathF_Log10)
ICALL(MATHF_23, "ModF(float,float&)", ves_icall_System_MathF_ModF)

This comment has been minimized.

@kumpera

kumpera Apr 2, 2018

Member

This signature doesn't match the managed signature.

This comment has been minimized.

@migueldeicaza

migueldeicaza Apr 2, 2018

Member

Good catch, thanks!

This comment has been minimized.

@migueldeicaza

migueldeicaza Apr 2, 2018

Member

I changed my mind, I kept the .NET signature.

This comment has been minimized.

@migueldeicaza

migueldeicaza Apr 2, 2018

Member

Short version: done

}
float
ves_icall_System_MathF_ModF (float x, float *d)

This comment has been minimized.

@kumpera

kumpera Apr 2, 2018

Member

This icall must null check d

This comment has been minimized.

@migueldeicaza

migueldeicaza Apr 2, 2018

Member

I changed this to be ref, do we still need the check in that case?

CoreCLR does not have that.

This comment has been minimized.

@kumpera

kumpera Apr 2, 2018

Member

If managed was using a byref instead of a pointer, we won't need it as we emit caller side checks. That doesn't happen with pointers.

This comment has been minimized.

@kumpera

kumpera Apr 2, 2018

Member

Looks like there's it's only used by Truncate and Round with locals, so it's just lousy code on their end but we should be safe.

This comment has been minimized.

@migueldeicaza

migueldeicaza Apr 2, 2018

Member

Added a check, just for sanity's sake.

@migueldeicaza

This comment has been minimized.

Member

migueldeicaza commented Apr 2, 2018

Mhm, the build breaks elsewhere with a series of errors like this:

/Users/builder/jenkins/workspace/test-mono-pull-request-amd64-osx/external/corefx/src/System.Numerics.Vectors/src/System/Numerics/Matrix3x2.cs(281,26): error CS0436: The type 'MathF' in '/Users/builder/jenkins/workspace/test-mono-pull-request-amd64-osx/mcs/class/System.Numerics/../../../external/corefx/src/Common/src/System/MathF.netstandard.cs' conflicts with the imported type 'MathF' in 'mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089'. Using the type defined in '/Users/builder/jenkins/workspace/test-mono-pull-request-amd64-osx/mcs/class/System.Numerics/../../../external/corefx/src/Common/src/System/MathF.netstandard.cs'.

migueldeicaza added some commits Apr 2, 2018

@luhenry

This comment has been minimized.

Member

luhenry commented Apr 3, 2018

@monojenkins build Linux x64

@luhenry

This comment has been minimized.

Member

luhenry commented Apr 3, 2018

@monojenkins build Linux AArch64 Interpreter

@luhenry

This comment has been minimized.

Member

luhenry commented Apr 3, 2018

@akoeplinger @marek-safar can you please check the API Diff

@akoeplinger

This comment has been minimized.

Member

akoeplinger commented Apr 4, 2018

The API diff looks fine to me.

@marek-safar

Please also add external/corefx/src/System.Runtime.Extensions/tests/System/MathTests.netcoreapp.cs to corlib_xtest.dll.sources to add tests which test the new APIs

{
public static partial class MathF
{
[MethodImpl(MethodImplOptions.InternalCall)]

This comment has been minimized.

@marek-safar

marek-safar Apr 4, 2018

Member

It'd be nice to follow Mono coding style

This comment has been minimized.

@migueldeicaza

migueldeicaza Apr 5, 2018

Member

I rather not for code that is copy pasted, makes it simpler to track changes.

@@ -978,6 +978,8 @@ ReferenceSources/Type.cs
../referencesource/mscorlib/system/invalidtimezoneexception.cs
../referencesource/mscorlib/system/Lazy.cs
../referencesource/mscorlib/system/math.cs
../../../external/corert//src/System.Private.CoreLib/shared/System/MathF.cs

This comment has been minimized.

@marek-safar

marek-safar Apr 4, 2018

Member

Please use version from ../../../external/corefx/src/Common/src/CoreLib/System/MathF.cs

This comment has been minimized.

@migueldeicaza
#if MONO
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static byte Clamp(byte value, byte min, byte max)
{

This comment has been minimized.

@marek-safar

marek-safar Apr 4, 2018

Member

I am tentative on this, would much rather see directly used ../external/corefx/src/Common/src/CoreLib/System/Math.cs but we could do it as follow up change

@marek-safar

This comment has been minimized.

Member

marek-safar commented Apr 5, 2018

The System.Runtime.Extensions change for a desktop could be problematic but let's see if anyone hits it

@marek-safar

This comment has been minimized.

Member

marek-safar commented Apr 5, 2018

@monojenkins commit apidiff

monojenkins added a commit to mono/api-snapshot that referenced this pull request Apr 5, 2018

@migueldeicaza migueldeicaza requested a review from luhenry as a code owner Apr 5, 2018

@akoeplinger akoeplinger merged commit 52df0f2 into mono:master Apr 5, 2018

3 of 23 checks passed

Linux AArch64 Build triggered for merge commit.
Details
Linux AArch64 Interpreter Build triggered for merge commit.
Details
Linux ARMv5 Build triggered for merge commit.
Details
Linux ARMv7 Build triggered for merge commit.
Details
Linux ARMv7 Interpreter Build triggered for merge commit.
Details
Linux i386 Build triggered for merge commit.
Details
Linux i386 Coop GC Build started for merge commit.
Details
Linux x64 Build started for merge commit.
Details
Linux x64 Acceptance Tests Build started for merge commit.
Details
Linux x64 Checked Private Types Build Build started for merge commit.
Details
Linux x64 Coop GC Build started for merge commit.
Details
Linux x64 FullAOT Build started for merge commit.
Details
Linux x64 Interpreter Build started for merge commit.
Details
Linux x64 mcs Build started for merge commit.
Details
OS X i386 Build triggered for merge commit.
Details
OS X x64 Build triggered for merge commit.
Details
OS X x64 Android SDK Build triggered for merge commit.
Details
OS X x64 iOS SDK Build triggered for merge commit.
Details
Windows i386 Build triggered for merge commit.
Details
Windows x64 Build triggered for merge commit.
Details
PR Trigger Docs Click to view available PR triggers (Xamarin internal).
Details
Test Result Viewer Click to view aggregated test results (Xamarin internal).
Details
license/cla All CLA requirements met.
Details

akoeplinger added a commit to akoeplinger/mono that referenced this pull request Apr 6, 2018

[metadata] Fix wrong icall signature for ModF
Noticed this while looking at Jenkins logs today:

```
15:00:56 /mnt/jenkins/workspace/test-mono-pull-request-amd64/mcs/class/lib/net_4_x/mscorlib.dll
15:00:56 cant resolve internal call to "System.MathF::ModF(single,single*)" (tested without signature also)
```

We need to use `single` in the icall def instead of `float`.

It was introduced by mono#7941

akoeplinger added a commit that referenced this pull request Apr 6, 2018

[metadata] Fix wrong icall signature for ModF (#8080)
Noticed this while looking at Jenkins logs today:

```
15:00:56 /mnt/jenkins/workspace/test-mono-pull-request-amd64/mcs/class/lib/net_4_x/mscorlib.dll
15:00:56 cant resolve internal call to "System.MathF::ModF(single,single*)" (tested without signature also)
```

We need to use `single` in the icall def instead of `float`.

It was introduced by #7941
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment