Moving parts of `System.Math` and `System.MathF` to be shared with CoreRT. #14119

tannergooding · 2017-09-21T20:30:39Z

This splits System.Math and System.MathF into partial classes so that parts of the code can be shared with CoreRT.

The changes on the native side are:

COMDouble and COMSingle now export a FMod function which wraps fmod
COMDouble and COMSingle now export a ModF function which matches the native signature
COMDouble and COMSingle no longer export a Round function

The changes on the managed side are:

A few more AggressiveInlining attributes, to match CoreRT
Round is now implemented in managed code
RoundInternal is directly inlined now
Truncate calls a new modf private function, rather than InternalTruncate which itself called SplitFraction
copysign is a new private function directly implemented in managed code (this supports Round)

Everything else is just moving the types around between the files or non functional changes such as:

Adding braces that were previously missing
Using a ternary expression (instead of if/else) when we are selecting between two return values
Using the keyword rather than full type name (this seems to be the more common and recommended behavior)

tannergooding · 2017-09-21T20:30:47Z

FYI. @jkotas

tannergooding · 2017-09-21T20:55:04Z

Also FYI. @mellinoe

jkotas · 2017-09-21T21:13:12Z

src/mscorlib/src/System/Math.cs

@@ -13,712 +13,73 @@



The convention has been to rename the CoreCLR specific part like Math.CoreCLR.cs.

jkotas · 2017-09-21T21:13:26Z

src/mscorlib/src/System/Math.cs

-            return div;
-        }
+        [MethodImpl(MethodImplOptions.InternalCall)]
+        private static extern double fmod(double x, double y);


Use CamelCase naming?

jkotas · 2017-09-21T21:15:17Z

src/vm/ecalllist.h

    FCIntrinsic("Pow", COMDouble::Pow, CORINFO_INTRINSIC_Pow)
-    FCIntrinsic("Round", COMDouble::Round, CORINFO_INTRINSIC_Round)


This means that Round will be no longer treated as intrinsic by the JIT.

What is the mechanism used to treat a non FCALL method as intrinsic?

Mark the method with [Intrinsic] attribute and recognize it by name in the JIT.

#13815 introduced this mechanism.

Alright, thanks!

I've already added the [Intrinsic] attribute, so I should just need to recognize it by name.

@jkotas, I ended up adding two new named intrinsics (one for Math.Round and one for MathF.Round).

I then modified the Compiler::impIntrinsic in importer to map the new named intrinsic to the old CORINFO intrinsic.

This looks to be working end to end right now, since the named intrinsic checks only happen in morph and importer, but only before the GenTree* has been created.

… intrinsic.

AndyAyersMS · 2017-09-22T04:59:49Z

src/jit/importer.cpp

+    // changes (such as if we support all methods in a class as intrinsic), we can reorder
+    // the checks to account for that.
+
+    if ((namespaceName != nullptr) && (className != nullptr) && (methodName != nullptr))


I'm fine with the reordering to put all the null checks up front. I have something similar in some of my pending changes.

However, I believe the jit will always be asking if a particular method is an intrinsic here. So I suggest the comment be removed or reworded.

Fixed to no longer include the part about reordering if it ever changes.

AndyAyersMS · 2017-09-22T05:01:58Z

src/jit/importer.cpp

+            case NI_Math_Round:
+            {
+                // Math.Round and MathF.Round used to be a legacy JIT intrinsic. In order
+                // to simplify the transition, we will just treat it as if it was still the


Nitpick: "legacy" has special meaning in the jit code base. Maybe say "used to be a traditional jit intrinsic."

Fixed to use 'traditional' rather than legacy.

AndyAyersMS · 2017-09-22T05:05:45Z

src/jit/importer.cpp

-            }
-
-            retNode = op1;
+            retNode = impMathIntrinsic(method, sig, callType, intrinsicID, tailCall);


We are still going to see CORINFO_INTRINISC_Round on desktop -- desktop does not have the new-style intrinsics.

So it needs to remain in here for now.

If that seems messy, you can hide it from CoreCLR builds with ifdefs, but I wouldn't bother.

@AndyAyersMS, just to confirm, removing the mapping from ecalllist (in the VM layer) should be fine, correct?

Not sure which parts of the codebase are shared with Desktop and which parts aren't.

removing the mapping from ecalllist (in the VM layer) should be fine, correct?

Yes.

Not sure which parts of the codebase are shared with Desktop and which parts aren't.

JIT and GC is.

AndyAyersMS

Left you some notes. The desktop compat one is the most serious.

…is required for Desktop compat.

tannergooding · 2017-09-22T15:00:40Z

All feedback given so far has been resolved. Please let me know if anything else needs to be changed.

tannergooding · 2017-09-22T19:53:22Z

Going to ping on this to see if it can get merged before EOD today, that way I can work on dotnet/corert#3167 and https://github.com/dotnet/corefx/issues/16428 this weekend.

jkotas · 2017-09-22T20:13:06Z

src/jit/importer.cpp

+    // Currently we only have intrinsics at the method level, so we can check that
+    // namespaceName, className, and methodName are all not null upfront.
+
+    if ((namespaceName != nullptr) && (className != nullptr) && (methodName != nullptr))


Nit: It would be nice to early out here to save an extra level of indention for the actual matching code that is likely going to grow pretty big.

jkotas · 2017-09-22T20:14:46Z

@dotnet-bot test Windows_NT x64 corefx_baseline
@dotnet-bot test Windows_NT x86 corefx_baseline
@dotnet-bot test Ubuntu x64 corefx_baseline

jkotas · 2017-09-22T20:16:25Z

The VM and CoreLib parts look fine to me - assuming the corefx tests that I have triggered pass.

@AndyAyersMS or somebody else on @dotnet/jit-contrib should sing off on the JIT part.

tannergooding · 2017-09-22T20:18:04Z

Thanks!

AndyAyersMS

Thanks for the updates. Looks good.

tannergooding · 2017-09-22T20:34:03Z

The Ubuntu job hasn't updated yet but it failed due to clang being missing:

13:29:18 Setting up directories for build
13:29:18 Checking pre-requisites...
13:29:18 Please install clang before running this script
13:29:18 Command execution failed with exit code 1.

tannergooding · 2017-09-22T20:41:46Z

Requeued the job and it seems to be actually going now (new machine).

The bad machine (missing clang) was: ubuntu1404-20170821-aec340 -- cc @mmitche

tannergooding · 2017-09-22T20:53:35Z

Actually, I take that back. The new run failed as well with the same issue:

13:51:17 Done initializing tools.
13:51:18 Running: /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/src/Native/build-native.sh   x64  Release  Linux  --numproc 4  -portable  toolSetDir=c:\tools\clr /p:CoreCLROverridePath=/mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/bin/Product/Linux.x64.Checked
13:51:18 Setting up directories for build
13:51:18 Checking pre-requisites...
13:51:18 Please install clang before running this script
13:51:18 Command execution failed with exit code 1.
13:51:18 [Current build status] check if current [SUCCESS] is worse or equals then [SUCCESS] and better or equals then [SUCCESS]

CC. @jkotas, it doesn't look to be related to my changes.

tannergooding · 2017-09-22T20:54:31Z

Its definitely strange since clang is found by a separate script at an early step in the build.

janvorli · 2017-09-22T21:43:52Z

I am sorry, the CoreFX tests in the CI for that change have passed, so I have assumed the VMs were fixed.

janvorli · 2017-09-22T22:29:04Z

@mikem8361, @tannergooding, @jkotas I am going to revert that change. It requires creating the revert manually though since github doesn't want to do it automatically.

tannergooding · 2017-09-22T22:38:58Z

@jkotas, looks like Windows_NT both failed in an unrelated area. System.Threading.Extensions.Tests failed due to FailFast: SetStateMachine should not be used.

tannergooding · 2017-09-22T22:39:35Z

It looks like all other tests passed.

janvorli · 2017-09-22T22:42:36Z

Revert PR is up: dotnet/corefx#24215

tannergooding · 2017-09-22T22:43:48Z

Windows_NT failure looks to be caused by https://github.com/dotnet/coreclr/blame/8c33e99150cfe9c173f524acff4ea7be320ce08c/src/mscorlib/src/System/Runtime/CompilerServices/AsyncMethodBuilder.cs#L351. Change was done by @stephentoub

stephentoub · 2017-09-22T23:13:38Z

looks like Windows_NT both failed in an unrelated area. System.Threading.Extensions.Tests

Which test?

stephentoub · 2017-09-22T23:18:12Z

Oh, I see, that test should be deleted. I'll do so. You can ignore the failure.

tannergooding · 2017-09-23T00:40:02Z

@dotnet-bot test Ubuntu x64 corefx_baseline

tannergooding · 2017-09-23T00:40:27Z

@dotnet-bot test Windows_NT x64 corefx_baseline
@dotnet-bot test Windows_NT x86 corefx_baseline

tannergooding · 2017-09-23T00:41:23Z

Both known blocking failures have been fixed. I've requeued the tests.

@jkotas, am I good to merge later tonight if all three runs come back green?

jkotas

Yes

jkotas · 2017-09-23T00:50:37Z

Could you please also rename src/mscorlib/src/System/Math.cs to src/mscorlib/src/System/Math.CoreCLR.cs ?

tannergooding · 2017-09-23T00:52:35Z

Could you please also rename src/mscorlib/src/System/Math.cs to src/mscorlib/src/System/Math.CoreCLR.cs ?

Done. Thought I had done this already.

tannergooding · 2017-09-23T00:52:49Z

@dotnet-bot test Windows_NT x64 corefx_baseline
@dotnet-bot test Windows_NT x86 corefx_baseline
@dotnet-bot test Ubuntu x64 corefx_baseline

tannergooding · 2017-09-23T04:27:07Z

Ubuntu is failing four of the FileSystemWatcherTests:

System.IO.Tests.FileSystemWatcherTests_netstandard17.EndInit_ResumesPausedEnableRaisingEvents
System.IO.Tests.FileSystemWatcherTests_netstandard17.EndInit_ResumesPausedEnableRaisingEvents(setBeforeBeginInit: True)
System.IO.Tests.FileSystemWatcherTests_netstandard17.EndInit_ResumesPausedEnableRaisingEvents(setBeforeBeginInit: False)
System.IO.Tests.File_Create_Tests.FileSystemWatcher_File_Create

I don't see these failing anywhere else, so I will investigate them tomorrow morning.

Looks like the failures are all that the test is expected some event to occur, but it is not receiving it

tannergooding · 2017-09-23T16:52:05Z

Ok, looks like the Ubuntu FileSystemWatcherTests failures are not unique to my PR. They are also failing in the corefx_baseline for current master. Ex: https://ci.dot.net/job/dotnet_coreclr/job/master/job/jitstress/job/x64_checked_ubuntu_corefx_baseline/298/

tannergooding · 2017-09-23T16:54:46Z

Its going to take a bit, but I'm going to try to pin down the commit that introduced the regression.

tannergooding · 2017-09-23T21:11:01Z

The Ubuntu FileSystemWatcherTests failures were introduced in #14043. -- CC. @stephentoub, @pjanotti, @JeremyKuhne

tannergooding · 2017-09-23T21:11:49Z

@jkotas, can this be merged?

tannergooding · 2017-09-23T21:16:30Z

Logged a bug for the issue here: https://github.com/dotnet/coreclr/issues/14154

tannergooding · 2017-09-23T21:28:09Z

Thanks!

tannergooding · 2017-09-25T16:16:02Z

@jkotas, it looks like a PR to mirror these changes has not gone up to CoreRT yet.

Given that there will be some fixup required there as well, could you tag me when it comes up?

jkotas · 2017-09-25T16:34:25Z

PR to mirror these changes has not gone up to CoreRT yet.

cc @safern

safern · 2017-09-25T16:38:21Z

Taking a look now.

dnfclas added the cla-already-signed label Sep 21, 2017

jkotas reviewed Sep 21, 2017

View reviewed changes

tannergooding added 4 commits September 21, 2017 20:02

Moving parts of System.MathF to be shared with CoreRT.

047d4ce

Moving parts of System.Math to be shared with CoreRT.

d38374d

Updating the new 'Round' named intrinsic to map to the legacy 'Round'…

687b880

… intrinsic.

Adding a message to the Math.Round and MathF.Round implementations

04fdd3d

tannergooding mentioned this pull request Sep 22, 2017

Fixing FloatingPointUtils.round to include the fix for #12137 #14133

Merged

Applying the code format patch for importer.cpp

76f2ba1

tannergooding mentioned this pull request Sep 22, 2017

JIT: devirtualization support for EqualityComparer<T>.Default #14125

Merged

AndyAyersMS reviewed Sep 22, 2017

View reviewed changes

AndyAyersMS suggested changes Sep 22, 2017

View reviewed changes

tannergooding added 2 commits September 21, 2017 22:07

Fixing up some comments in importer.cpp

ba7ddb9

Adding back the case statement for CORINFO_INTRINSIC_Round, since it …

86e8913

…is required for Desktop compat.

jkotas reviewed Sep 22, 2017

View reviewed changes

AndyAyersMS approved these changes Sep 22, 2017

View reviewed changes

jkotas approved these changes Sep 23, 2017

View reviewed changes

Renaming the non-shared copy of Math.cs to Math.CoreCLR.cs

eebceeb

jkotas merged commit bfaad96 into dotnet:master Sep 23, 2017

tannergooding mentioned this pull request Nov 7, 2017

Enable CORINFO_INTRINSIC Round, Ceiling, and Floor to generate ROUNDSS and ROUNDSD #14736

Merged

tannergooding deleted the shared-math branch January 17, 2018 01:55

tannergooding mentioned this pull request Jan 31, 2020

Several of the FileSystemWatcherTests are failing after PR 14043 dotnet/runtime#9000

Closed

		FCIntrinsic("Pow", COMDouble::Pow, CORINFO_INTRINSIC_Pow)
		FCIntrinsic("Round", COMDouble::Round, CORINFO_INTRINSIC_Round)

Moving parts of System.Math and System.MathF to be shared with CoreRT. #14119

Moving parts of System.Math and System.MathF to be shared with CoreRT. #14119

Conversation

tannergooding commented Sep 21, 2017

tannergooding commented Sep 21, 2017

tannergooding commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS left a comment

Choose a reason for hiding this comment

tannergooding commented Sep 22, 2017

tannergooding commented Sep 22, 2017

Choose a reason for hiding this comment

jkotas commented Sep 22, 2017

jkotas commented Sep 22, 2017

tannergooding commented Sep 22, 2017

AndyAyersMS left a comment

Choose a reason for hiding this comment

tannergooding commented Sep 22, 2017

tannergooding commented Sep 22, 2017

tannergooding commented Sep 22, 2017

tannergooding commented Sep 22, 2017

janvorli commented Sep 22, 2017

janvorli commented Sep 22, 2017

tannergooding commented Sep 22, 2017

tannergooding commented Sep 22, 2017

janvorli commented Sep 22, 2017

tannergooding commented Sep 22, 2017

stephentoub commented Sep 22, 2017

stephentoub commented Sep 22, 2017

tannergooding commented Sep 23, 2017

tannergooding commented Sep 23, 2017

tannergooding commented Sep 23, 2017

jkotas left a comment

Choose a reason for hiding this comment

jkotas commented Sep 23, 2017

tannergooding commented Sep 23, 2017

tannergooding commented Sep 23, 2017

tannergooding commented Sep 23, 2017 • edited Loading

tannergooding commented Sep 23, 2017

tannergooding commented Sep 23, 2017

tannergooding commented Sep 23, 2017

tannergooding commented Sep 23, 2017

tannergooding commented Sep 23, 2017

tannergooding commented Sep 23, 2017

tannergooding commented Sep 25, 2017

jkotas commented Sep 25, 2017

safern commented Sep 25, 2017

Moving parts of `System.Math` and `System.MathF` to be shared with CoreRT. #14119

Moving parts of `System.Math` and `System.MathF` to be shared with CoreRT. #14119

tannergooding commented Sep 23, 2017 •

edited

Loading