-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Moving Math.Abs(double)
and Math.Abs(float)
to be implemented in managed code.
#14156
Conversation
The managed implementation of test Windows_NT_x64 perf |
FYI. @mellinoe |
@dotnet-bot help please |
@dotnet-bot test Windows_NT x64 perf |
This will make these methods slower when they are not expanded as intrinsics. Related to my comment https://github.com/dotnet/coreclr/issues/14155#issuecomment-331686439. |
@jkotas, I don't believe that is the case and does not match the numbers I was seeing locally (will try to get actual numbers and post sometime tomorrow). The managed implementation should get inlined by the JIT, where-as the FCALL (when not compiled to an intrinsic) is simple enough that the resulting call ends up removing any benefits that the slightly better codegen the CRT implementation had. |
Have you tried what happens when somebody calls this via delegate? |
I will be sure to include this in the numbers I collect. |
The following are from a local run of the Math Functions benchmark (1000 outer iterations, 5000 inner iterations) for the
I have ordered these result from least time to greatest time:
Notes:
So I think, at the very least, there are some x86 codegen bugs that need to be filed. |
I expect that you will get similar results for all platforms that did not get as much codegen investments as x64. Fallback to C implementation is a better option for these platforms. I think that the Math methods should be wired to use the default C implementation by default. The managed or hardware intrinsic implementation should be opt-in per platform - for platforms where we have deeper codegen investments that allows us to do better than the C implementation. This strategy should work well for all Math methods. Abs is outlier because of it is simpler and you can be more creative with it, but I do not think that it is a good reason for inventing different scheme for it. |
@jkotas, wiring to the C runtime implementation for the platform is "easy", but has a high chance of introducing various bugs, performance differences, input/output differences, etc (as has already been found just from bringing Linux and OSX on-board). Additionally, the last Math API review meeting (https://github.com/dotnet/corefx/issues/16428) made it fairly clear that we did not want to continue bloating corlib with more math APIs and instead wanted to do something like provide them in a separate library where they are implemented in managed code using hardware intrinsics with a software fallback. What is your proposal/thoughts to resolving these issues to ensure that the underlying implementations are consistent, performant, easily updatable, etc? To me, it still seems like the best way forward is to (for all Math and MathF methods):
From what I can tell, this is roughly what we are already doing in CoreRT (although the pure software implementation is a P/Invoke into the C Runtime for most methods). This is definitely what For reference, some of the currently tracked bugs (at least that I filed) are:
I know other users have logged others bugs in similar areas as well. Additionally, some users have expressed want for I believe this would, as well, be most easily provided through a managed implementation with runtime knowledge of the special switches (if such a feature would be approved and provided). |
There have also been several bugs that have already been fixed/resolved that I did not list. With this code being based on the underlying C Runtime, rather than being in shared managed code, it also means that each runtime (CoreCLR, CoreRT, etc) each has to be updated and kept in sync manually when a workaround is added (and this is easily overlooked or forgotten). For example, while moving the non-extern methods in |
The important thing is that the "software based implementation in managed code" is called when neither 2 or 3 are provided... we surely are interested in Cosmos to have SSE intrinsic supported but not for now. |
This is part of https://github.com/dotnet/coreclr/issues/14155.