Add managed implementation of Math.ILogB and MathF.ILogB #56236

alexcovington · 2021-07-23T17:21:38Z

This PR adds a managed implementation of Math.ILogB and MathF.ILogB based on the musl implementation.

BenchmarkDotNet results:

// * Summary *

BenchmarkDotNet=v0.13.0.1559-nightly, OS=Windows 10.0.19042.1110 (20H2/October2020Update)
AMD Ryzen 7 5800, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.100-rc.1.21372.14
  [Host]     : .NET 6.0.0 (6.0.21.37201), X64 RyuJIT
  Job-RIMCAC : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
  Job-KUXINN : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT

PowerPlanMode=00000000-0000-0000-0000-000000000000  Arguments=/p:DebugType=portable  IterationTime=250.0000 ms
MaxIterationCount=20  MinIterationCount=15  WarmupCount=1

|   Type | Method |        Job |                                                                                                        Toolchain |     Mean |    Error |   StdDev |   Median |      Min |      Max | Ratio | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------- |------- |----------- |----------------------------------------------------------------------------------------------------------------- |---------:|---------:|---------:|---------:|---------:|---------:|------:|------:|------:|------:|----------:|
| Double |  ILogB | Job-RIMCAC | \runtime-master\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 30.73 us | 0.017 us | 0.015 us | 30.72 us | 30.71 us | 30.75 us |  1.00 |     - |     - |     - |         - |
| Double |  ILogB | Job-KUXINN |        \runtime\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 11.46 us | 0.004 us | 0.003 us | 11.46 us | 11.46 us | 11.47 us |  0.37 |     - |     - |     - |         - |
|        |        |            |                                                                                                                  |          |          |          |          |          |          |       |       |       |       |           |
| Single |  ILogB | Job-RIMCAC | \runtime-master\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 30.60 us | 0.009 us | 0.008 us | 30.60 us | 30.59 us | 30.62 us |  1.00 |     - |     - |     - |         - |
| Single |  ILogB | Job-KUXINN |        \runtime\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12.90 us | 0.008 us | 0.007 us | 12.90 us | 12.89 us | 12.91 us |  0.42 |     - |     - |     - |         - |

Please let me know if I can clarify anything. Thanks!

…ion in ecallist and vm

ghost · 2021-07-23T17:21:42Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

EgorBo · 2021-07-23T17:49:15Z

Nice work! I suspect for Mono-interp in general it's better to have libc calls rather than long C# implementations, but I'm not sure how popular ILogB is in mono-interp related workloads.

Do we have enough tests to cover all the edge cases?

alexcovington · 2021-07-23T17:53:54Z

@EgorBo I see, I could put a guard around the implementation to just make the change for CoreCLR if you think that would be better. Do you know if there is a TARGET_CORECLR or equivalent macro?

I looked at the current test cases, and it seems like there should be enough. I will double check the musl test cases as well to see if there are any others that could be added.

tannergooding · 2021-07-23T17:54:28Z

src/libraries/System.Private.CoreLib/src/System/Math.cs

            }
        }

+        public static int ILogB(double x)


There is some additional handling in src/coreclr/jit/valuenum.cpp that supports constant folding. We need to ensure that stays in sync with the managed implementation.

I'm unsure if there is a trivial way to get value numbering to defer to the managed implementation or if instead we need to maintain two copies of the code. @jkotas or @briansull may be able to help on this part.

It should be possible to defer to the managed implementation (for both JIT and AOT cases). It will require a new method on JIT/EE interface.

Just as a summary...

With this change, we'll have 3 System.Math/MathF methods implemented in managed code:

Round

ScaleB

ILogB

The first has a corresponding native implementation with CSE support, while the second only has a managed implementation and no CSE support.

One of the main concerns with moving all methods to managed is long term maintenance cost, especially around more complex methods like sin or cos. Hence we've done minimal investigation around having a single native implementation for all platforms taken from something like https://github.com/amd/aocl-libm-ose.

However, we do have some methods (like ILogB, ScaleB, and Round) that are "sufficiently simple" that implementing in managed code is probably worthwhile. It would be nice to ensure that we consistently handle them in CSE when we do move them to managed so we don't lose out on any existing optimizations and so we don't need to maintain 2 or more copies of the logic.

Note that for CSE logic specifically you do not need to do anything special w.r.t. to where and how the method is implemented - you can have it be an opague VNFunc and it'll work just fine (this is how a few cast helpers are implemented today).

The only place where the actual implementation matters is VN (or not VN) folding.

Round, ScaleB, ILogB

Are typical implementations of these methods 100% precise, or are they approximations? I am thinking we really only need to solve this issue once we get to the managed implementations of methods that are implemented as approximation (sin, cos, etc.)

Round is 100% precise.

I'm unsure of the guarantees of ScaleB or ILogB that we have, but I would expect these two in particular to be 100% precise as well.

The managed ScaleB and ILogB implementations are based off the musl implementations, which are 100% precise to my knowledge.

I was unaware that CSE might treat these functions differently, please let me know if there are any other changes I can make to help keep the implementations consistent.

A more precise statement would be that we fold them in VN and thus need the native ilogb to always match the managed version. If both are precise (i. e. always return the same value for the same input), then we need not worry about it.

@SingleAccretion Thanks, that makes more sense. Since the musl implementations are 100% precise, I don't expect any mismatches.

Please let me know if there is anything else I can look at.

jkotas · 2021-07-25T05:23:41Z

Mono-interp in general it's better to have libc calls rather than long C# implementations

We do not optimize the runtime libraries for mono interpreter like this. We assume that performance critical methods will be AOT compiled in environments that do not allow JIT.

am11 · 2021-07-25T20:22:01Z

src/mono/System.Private.CoreLib/src/System/Math.Mono.cs

        [MethodImpl(MethodImplOptions.InternalCall)]
        public static extern double FusedMultiplyAdd(double x, double y, double z);

-        [MethodImpl(MethodImplOptions.InternalCall)]


can we clean these from sysmath.c, interp.c, mintops.def, icall-def-netcore.h and transform.c too?

Absolutely, I've removed ILogB logic from those files. Please let me know if there is anything else I can modify.

Great, thanks!

git stats are just perfect! 👌😅

ghost · 2021-08-30T16:10:05Z

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

Issue Details

This PR adds a managed implementation of Math.ILogB and MathF.ILogB based on the musl implementation.

BenchmarkDotNet results:

// * Summary *

BenchmarkDotNet=v0.13.0.1559-nightly, OS=Windows 10.0.19042.1110 (20H2/October2020Update)
AMD Ryzen 7 5800, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.100-rc.1.21372.14
  [Host]     : .NET 6.0.0 (6.0.21.37201), X64 RyuJIT
  Job-RIMCAC : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT
  Job-KUXINN : .NET 6.0.0 (42.42.42.42424), X64 RyuJIT

PowerPlanMode=00000000-0000-0000-0000-000000000000  Arguments=/p:DebugType=portable  IterationTime=250.0000 ms
MaxIterationCount=20  MinIterationCount=15  WarmupCount=1

|   Type | Method |        Job |                                                                                                        Toolchain |     Mean |    Error |   StdDev |   Median |      Min |      Max | Ratio | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------- |------- |----------- |----------------------------------------------------------------------------------------------------------------- |---------:|---------:|---------:|---------:|---------:|---------:|------:|------:|------:|------:|----------:|
| Double |  ILogB | Job-RIMCAC | \runtime-master\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 30.73 us | 0.017 us | 0.015 us | 30.72 us | 30.71 us | 30.75 us |  1.00 |     - |     - |     - |         - |
| Double |  ILogB | Job-KUXINN |        \runtime\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 11.46 us | 0.004 us | 0.003 us | 11.46 us | 11.46 us | 11.47 us |  0.37 |     - |     - |     - |         - |
|        |        |            |                                                                                                                  |          |          |          |          |          |          |       |       |       |       |           |
| Single |  ILogB | Job-RIMCAC | \runtime-master\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 30.60 us | 0.009 us | 0.008 us | 30.60 us | 30.59 us | 30.62 us |  1.00 |     - |     - |     - |         - |
| Single |  ILogB | Job-KUXINN |        \runtime\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.0\corerun.exe | 12.90 us | 0.008 us | 0.007 us | 12.90 us | 12.89 us | 12.91 us |  0.42 |     - |     - |     - |         - |

Please let me know if I can clarify anything. Thanks!

Author:	alexcovington
Assignees:	-
Labels:	`area-System.Runtime`, `community-contribution`
Milestone:	-

tannergooding · 2021-08-30T16:15:51Z

src/libraries/System.Private.CoreLib/src/System/Math.cs


+        public static int ILogB(double x)
+        {
+            // Implementation based on https://github.com/ifduyue/musl/blob/cfdfd5ea3ce14c6abf7fb22a531f3d99518b5a1b/src/math/ilogb.c


Can we switch this to the official MUSL source: https://git.musl-libc.org/cgit/musl/tree/src/math/ilogb.c ?

At first glance, they appear to be identical and it would be better, IMO, to ensure we use the main source rather than a fork.

Sure, makes sense. I've updated the citations, please let me know if there are any other changes I can make.

tannergooding

This LGTM now that main is open for .NET 7 changes.

@dotnet/jit-contrib, @EgorBo could this get a review/sign-off from the JIT side.

Once this gets the JIT side sign-off, it can be merged.

…-ilogb

BruceForstall · 2021-09-18T00:31:51Z

@dotnet/jit-contrib, @EgorBo could this get a review/sign-off from the JIT side.

@tannergooding There are no JIT changes in this PR. What do you want a JIT dev to look at?

tannergooding · 2021-09-18T01:41:22Z

@BruceForstall, it was initially expected that there was going to be a ValueNum related change (https://github.com/dotnet/runtime/pull/56236/files#r675738468). It was determined that this was likely unnecessary since both the C and C# implementations should return identical results in all cases.

It would still be good for someone from the JIT to confirm there is no additional handling required/missed given the special casing that exists for the math intrinsics at various JIT stages.
-- A managed/native split isn't common for the Math APIs, Round is one of the few cases that this exists. Others are either fully managed or fully native and only plug into things like ValueNum if they are native.

kunalspathak · 2021-10-26T15:44:00Z

Ubuntu/x64 improvements in ILogB - dotnet/perf-autofiling-issues#1982

kunalspathak · 2021-10-26T15:47:25Z

another one - dotnet/perf-autofiling-issues#1988

alexcovington added 5 commits July 22, 2021 14:41

Added managed implementations for ILogB, removed extern bindings

716ce9e

Merge remote-tracking branch 'origin/main' into managed-ilogb

3e36c4f

Fix bug with managed MathF implementation, removing ILog2 implementat…

8d24d69

…ion in ecallist and vm

Add citations to musl implementation.

29f5b9c

Remove whitespace

11bb718

ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 23, 2021

Remove Mono bindings

821d09f

alexcovington requested a review from marek-safar as a code owner July 23, 2021 17:41

tannergooding reviewed Jul 23, 2021

View reviewed changes

Add more test cases from musl test library

9661c91

am11 reviewed Jul 25, 2021

View reviewed changes

Remove native ILogB logic from Mono

8abfd37

alexcovington requested review from BrzVlad, lambdageek, thaystg and vargaz as code owners July 26, 2021 16:56

marek-safar added the area-System.Runtime label Aug 30, 2021

tannergooding reviewed Aug 30, 2021

View reviewed changes

tannergooding approved these changes Aug 30, 2021

View reviewed changes

alexcovington added 2 commits August 30, 2021 09:21

Update citations to official MUSL source

113df08

Merge branch 'main' of https://github.com/dotnet/runtime into managed…

c3d9ef4

…-ilogb

jeffhandley assigned tannergooding Oct 19, 2021

tannergooding merged commit 838fed9 into dotnet:main Oct 19, 2021

ghost locked as resolved and limited conversation to collaborators Nov 25, 2021

Add managed implementation of Math.ILogB and MathF.ILogB #56236

Add managed implementation of Math.ILogB and MathF.ILogB #56236

Uh oh!

Conversation

alexcovington commented Jul 23, 2021

Uh oh!

ghost commented Jul 23, 2021

Uh oh!

EgorBo commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcovington commented Jul 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Jul 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jul 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding left a comment

Choose a reason for hiding this comment

Uh oh!

BruceForstall commented Sep 18, 2021

Uh oh!

tannergooding commented Sep 18, 2021

Uh oh!

kunalspathak commented Oct 26, 2021

Uh oh!

kunalspathak commented Oct 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

EgorBo commented Jul 23, 2021 •

edited

Loading

SingleAccretion Jul 25, 2021 •

edited

Loading