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.

Sign up for GitHub

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

Add managed implementation of Math/MathF.Abs #63881

Merged
merged 4 commits into from
Jan 19, 2022
Merged

Add managed implementation of Math/MathF.Abs #63881

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
386435e
Add managed implementation of Math/MathF.Abs
am11 Jan 17, 2022
97b4006
Cleanup trailing whitespaces in changeset
am11 Jan 17, 2022
cb293ad
Address CR feedback
am11 Jan 17, 2022
a083829
Delete unused _copysignf macro
am11 Jan 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions src/coreclr/System.Private.CoreLib/src/System/Math.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,6 @@ namespace System
{
public static partial class Math
{
[Intrinsic]
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern double Abs(double value);

[Intrinsic]
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern float Abs(float value);

[Intrinsic]
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern double Acos(double d);
Expand Down
9 changes: 0 additions & 9 deletions src/coreclr/classlibnative/float/floatdouble.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,6 @@
#pragma float_control(precise, off)
#endif

/*=====================================Abs======================================
**
==============================================================================*/
FCIMPL1_V(double, COMDouble::Abs, double x)
FCALL_CONTRACT;

return fabs(x);
FCIMPLEND

/*=====================================Acos=====================================
**
==============================================================================*/
Expand Down
9 changes: 0 additions & 9 deletions src/coreclr/classlibnative/float/floatsingle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,6 @@
#pragma float_control(precise, off)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I have noticed that there is unused #define _copysignf (float)_copysign a few lines above. It is left-over from times when CopySign was FCall. You delete it while you are on it.

#endif

/*=====================================Abs=====================================
**
==============================================================================*/
FCIMPL1_V(float, COMSingle::Abs, float x)
FCALL_CONTRACT;

return fabsf(x);
FCIMPLEND

/*=====================================Acos=====================================
**
==============================================================================*/
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/classlibnative/inc/floatdouble.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

class COMDouble {
public:
FCDECL1_V(static double, Abs, double x);
FCDECL1_V(static double, Acos, double x);
FCDECL1_V(static double, Acosh, double x);
FCDECL1_V(static double, Asin, double x);
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/classlibnative/inc/floatsingle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

class COMSingle {
public:
FCDECL1_V(static float, Abs, float x);
FCDECL1_V(static float, Acos, float x);
FCDECL1_V(static float, Acosh, float x);
FCDECL1_V(static float, Asin, float x);
Expand Down
12 changes: 0 additions & 12 deletions src/coreclr/nativeaot/System.Private.CoreLib/src/System/Math.CoreRT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,6 @@ namespace System
{
public static partial class Math
{
[Intrinsic]
public static float Abs(float value)
{
return RuntimeImports.fabsf(value);
}

[Intrinsic]
public static double Abs(double value)
{
return RuntimeImports.fabs(value);
}

[Intrinsic]
public static double Acos(double d)
{
Expand Down
12 changes: 0 additions & 12 deletions src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,18 +679,6 @@ internal struct ConservativelyReportedRegionDesc
[RuntimeImport(RuntimeLibrary, "RhpMemoryBarrier")]
internal static extern void MemoryBarrier();

[Intrinsic]
[MethodImplAttribute(MethodImplOptions.InternalCall)]
[RuntimeImport(RuntimeLibrary, "fabs")]
internal static extern double fabs(double x);

[Intrinsic]
internal static float fabsf(float x)
{
// fabsf is not a real export for some architectures
return (float)fabs(x);
}

[Intrinsic]
[MethodImplAttribute(MethodImplOptions.InternalCall)]
[RuntimeImport(RuntimeLibrary, "acos")]
Expand Down
2 changes: 0 additions & 2 deletions src/coreclr/vm/ecalllist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,8 +322,6 @@ FCFuncStart(gDelegateFuncs)
FCFuncEnd()

FCFuncStart(gMathFuncs)
FCFuncElementSig("Abs", &gsig_SM_Dbl_RetDbl, COMDouble::Abs)
FCFuncElementSig("Abs", &gsig_SM_Flt_RetFlt, COMSingle::Abs)
FCFuncElement("Acos", COMDouble::Acos)
FCFuncElement("Acosh", COMDouble::Acosh)
FCFuncElement("Asin", COMDouble::Asin)
Expand Down
28 changes: 26 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Math.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ public static partial class Math

private const double SCALEB_C3 = 9007199254740992; // 0x1p53

private const int ILogB_NaN = 0x7fffffff;
private const int ILogB_NaN = 0x7FFFFFFF;

private const int ILogB_Zero = (-1 - 0x7fffffff);
private const int ILogB_Zero = (-1 - 0x7FFFFFFF);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static short Abs(short value)
Expand Down Expand Up @@ -133,6 +133,30 @@ public static decimal Abs(decimal value)
return decimal.Abs(value);
}

[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static double Abs(double value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes overall look correct to me. However, I'd like to better understand the need for this change as I wouldn't expect this to provide any diffs for existing platforms or even make new platform bring up easier.

On RyuJIT Abs is [Intrinsic] today and so the actual CRT call will essentially never happen (modulo for indirect calls; such as function pointers, delegates, or reflection). We will always emit the correct assembly sequence instead; namely what is effectively Sse.And(Vector128.CreateScalarUnsafe(value), Vector128.CreateScalarUnsafe(0x7FFFFFFF).AsSingle()).ToScalar() on x86/x64 and what is effectively AdvSimd.Abs(Vector64.CreateScalarUnsafe(value)).ToScalar() on ARM64. This compiles down to a single instruction in both cases.

I would expect that Mono JIT, Mono LLVM, and NativeAOT are doing relatively the same, but if not I would expect a different PR rather than one updating the managed implementation here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this PR in the same spirit as ILogB change which went in earlier for .NET 7. My understanding was that we generally want to reduce number of FCalls, especially when it has zero cost.

or even make new platform bring up easier.

Do you mean it will rather make new platform bring up harder? Hard for me to imagine how, since this managed implementation is using a few BitConverter methods, which are also used by existing methods in Math.cs.

Copy link
Member

@EgorBo EgorBo Jan 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mono JIT (mini), Interp and LLVM all handle Abs as intrinsic (see MINT_ABS, OP_ABS and INTRINS_FABS). I guess NativeAOT is also fine since it uses RyuJIT.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean it will rather make new platform bring up harder?

No, I was just indicating that, as it appears to me, this is effectively a zero-diff PR (at least for expected real world use-cases).

That is, this PR doesn't make the code easier to read or maintain, it doesn't make new platform bring up easier, etc. The one place it will impact is indirect-calls, that is calls to Math.Abs that happen via delegates, function pointers, or reflection; but the overhead of the indirection is going to far out weigh the FCALL to native and so it's not even clear that it's beneficial for that niche case. If we were to care about indirect calls, I'd actually expect a different implementation, likely just using recursion like we do in HWIntrinsics to ensure "optimal" codegen.

{
// Implementation based on https://git.musl-libc.org/cgit/musl/tree/src/math/fabs.c
am11 marked this conversation as resolved.
Show resolved Hide resolved

const ulong signMask = 0x7FFFFFFFFFFFFFFF;
am11 marked this conversation as resolved.
Show resolved Hide resolved
am11 marked this conversation as resolved.
Show resolved Hide resolved
ulong raw = BitConverter.DoubleToUInt64Bits(value);

return BitConverter.UInt64BitsToDouble(raw & signMask);
}

[Intrinsic]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static float Abs(float value)
{
// Implementation based on https://git.musl-libc.org/cgit/musl/tree/src/math/fabsf.c

const uint signMask = 0x7FFFFFFF;
uint raw = BitConverter.SingleToUInt32Bits(value);

return BitConverter.UInt32BitsToSingle(raw & signMask);
}

[DoesNotReturn]
[StackTraceHidden]
private static void ThrowAbsOverflow()
Expand Down
6 changes: 0 additions & 6 deletions src/mono/System.Private.CoreLib/src/System/Math.Mono.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,6 @@ namespace System
{
public partial class Math
{
[MethodImpl(MethodImplOptions.InternalCall)]
public static extern double Abs(double value);

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern float Abs(float value);

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern double Acos(double d);

Expand Down
2 changes: 0 additions & 2 deletions src/mono/mono/metadata/icall-decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ ICALL_EXPORT void ves_icall_System_ArgIterator_Setup (MonoArgIterator*, char*, c
ICALL_EXPORT MonoType* ves_icall_System_ArgIterator_IntGetNextArgType (MonoArgIterator*);
ICALL_EXPORT void ves_icall_System_ArgIterator_IntGetNextArg (MonoArgIterator*, MonoTypedRef*);
ICALL_EXPORT void ves_icall_System_ArgIterator_IntGetNextArgWithType (MonoArgIterator*, MonoTypedRef*, MonoType*);
ICALL_EXPORT double ves_icall_System_Math_Abs_double (double);
ICALL_EXPORT double ves_icall_System_Math_Acos (double);
ICALL_EXPORT double ves_icall_System_Math_Acosh (double);
ICALL_EXPORT double ves_icall_System_Math_Asin (double);
Expand Down Expand Up @@ -116,7 +115,6 @@ ICALL_EXPORT float ves_icall_System_MathF_Sinh (float);
ICALL_EXPORT float ves_icall_System_MathF_Sqrt (float);
ICALL_EXPORT float ves_icall_System_MathF_Tan (float);
ICALL_EXPORT float ves_icall_System_MathF_Tanh (float);
ICALL_EXPORT float ves_icall_System_Math_Abs_single (float);
ICALL_EXPORT double ves_icall_System_Math_Log2 (double);
ICALL_EXPORT double ves_icall_System_Math_FusedMultiplyAdd (double, double, double);
ICALL_EXPORT float ves_icall_System_MathF_Log2 (float);
Expand Down
4 changes: 1 addition & 3 deletions src/mono/mono/metadata/icall-def-netcore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,7 @@ ICALL_TYPE(STREAM, "System.IO.Stream", STREAM_1)
HANDLES(STREAM_1, "HasOverriddenBeginEndRead", ves_icall_System_IO_Stream_HasOverriddenBeginEndRead, MonoBoolean, 1, (MonoObject))
HANDLES(STREAM_2, "HasOverriddenBeginEndWrite", ves_icall_System_IO_Stream_HasOverriddenBeginEndWrite, MonoBoolean, 1, (MonoObject))

ICALL_TYPE(MATH, "System.Math", MATH_19)
NOHANDLES(ICALL(MATH_19, "Abs(double)", ves_icall_System_Math_Abs_double))
NOHANDLES(ICALL(MATH_20, "Abs(single)", ves_icall_System_Math_Abs_single))
ICALL_TYPE(MATH, "System.Math", MATH_1)
NOHANDLES(ICALL(MATH_1, "Acos", ves_icall_System_Math_Acos))
NOHANDLES(ICALL(MATH_1a, "Acosh", ves_icall_System_Math_Acosh))
NOHANDLES(ICALL(MATH_2, "Asin", ves_icall_System_Math_Asin))
Expand Down
50 changes: 19 additions & 31 deletions src/mono/mono/metadata/sysmath.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* \file
* these are based on bob smith's csharp routines
* these are based on bob smith's csharp routines
*
* Author:
* Mono Project (http://www.mono-project.com)
Expand Down Expand Up @@ -55,49 +55,49 @@ ves_icall_System_Math_ModF (gdouble x, gdouble *d)
return modf (x, d);
}

gdouble
gdouble
ves_icall_System_Math_Sin (gdouble x)
{
return sin (x);
}

gdouble
gdouble
ves_icall_System_Math_Cos (gdouble x)
{
return cos (x);
}

gdouble
gdouble
ves_icall_System_Math_Cbrt (gdouble x)
{
return cbrt (x);
}

gdouble
gdouble
ves_icall_System_Math_Tan (gdouble x)
{
return tan (x);
}

gdouble
gdouble
ves_icall_System_Math_Sinh (gdouble x)
{
return sinh (x);
}

gdouble
gdouble
ves_icall_System_Math_Cosh (gdouble x)
{
return cosh (x);
}

gdouble
gdouble
ves_icall_System_Math_Tanh (gdouble x)
{
return tanh (x);
}

gdouble
gdouble
ves_icall_System_Math_Acos (gdouble x)
{
return acos (x);
Expand All @@ -109,78 +109,66 @@ ves_icall_System_Math_Acosh (gdouble x)
return acosh (x);
}

gdouble
gdouble
ves_icall_System_Math_Asin (gdouble x)
{
return asin (x);
}

gdouble
gdouble
ves_icall_System_Math_Asinh (gdouble x)
{
return asinh (x);
}

gdouble
gdouble
ves_icall_System_Math_Atan (gdouble x)
{
return atan (x);
}

gdouble
gdouble
ves_icall_System_Math_Atan2 (gdouble y, gdouble x)
{
return atan2 (y, x);
}

gdouble
gdouble
ves_icall_System_Math_Atanh (gdouble x)
{
return atanh (x);
}

gdouble
gdouble
ves_icall_System_Math_Exp (gdouble x)
{
return exp (x);
}

gdouble
gdouble
ves_icall_System_Math_Log (gdouble x)
{
return log (x);
}

gdouble
gdouble
ves_icall_System_Math_Log10 (gdouble x)
{
return log10 (x);
}

gdouble
gdouble
ves_icall_System_Math_Pow (gdouble x, gdouble y)
{
return pow (x, y);
}

gdouble
gdouble
ves_icall_System_Math_Sqrt (gdouble x)
{
return sqrt (x);
}

gdouble
ves_icall_System_Math_Abs_double (gdouble v)
{
return fabs (v);
am11 marked this conversation as resolved.
Show resolved Hide resolved
}

float
ves_icall_System_Math_Abs_single (float v)
{
return fabsf (v);
}

gdouble
ves_icall_System_Math_Ceiling (gdouble v)
{
Expand Down
Loading