Make RyuJIT/x86 the default x86 JIT by BruceForstall · Pull Request #8260 · dotnet/coreclr

BruceForstall · 2016-11-23T00:45:41Z

JIT32 becomes compatjit.dll and RyuJIT LEGACY_BACKEND becomes legacyjit.dll
(and is an altjit).

If JIT32 is not being built, then RyuJIT LEGACY_BACKEND becomes compatjit.dll
and is a normal jit (not an altjit).

Both clrjit.dll and compatjit.dll are added to the JIT NuGet package.

BruceForstall · 2016-11-23T00:45:56Z

@dotnet/jit-contrib PTAL

seanshpark · 2016-11-23T07:32:13Z

Hi, I'm currenrly working on to enable x86 on Linux. Can RyuJIT work also on Linux?
If not, should I modify the changes, may be later, to use Legacy when target is not Windows?

JosephTremoulet · 2016-11-23T13:47:15Z

LEGACY_BACKEND ... is a normal jit (not an altjit)

I'm a bit confused... once this merges, is the idea that you always leave COMPlus_AltJit unset to use Ryujit?

RussKeldorph · 2016-11-23T14:42:09Z

@JosephTremoulet Yes, this makes CoreCLR/x86 behave more like CoreCLR/x64, where by default you get RyuJIT. Unlike CoreCLR/x64, though, for the time being, you will be able to opt in to an older JIT with COMPlus_UseWindowsX86CoreLegacyJit=1, and that JIT will be the Legacy Backend JIT if built from these sources or JIT32 if using the official packages.

RussKeldorph · 2016-11-23T14:55:17Z

@seanshpark Great to hear you're working on x86 on Linux! RyuJIT/x86 isn't yet supported on Linux, so we can't comment on how well it works. If you have something working now, and this change breaks it, feel free to suggest changes or follow up with a subsequent PR yourself to revert to Legacy for Linux. Note that our assumption is that when CoreCLR/x86 comes to Linux, RyuJIT will be its JIT, so we certainly wouldn't object to you pushing forward with RyuJIT.

michellemcdaniel · 2016-11-23T15:50:15Z

Does this change update the lab jobs correctly such that the legacy_backend jobs pickup legacyjit.dll and the ryujit jobs pick up the correct jit? I assume that with this change COMPlus_AltJit will just be ignored? If not, we'll want to make sure that we update the jobs in the same PR. If we are picking up the correct jits, then I guess it would be ok to update the job definitions as a post step.

BruceForstall · 2016-11-23T16:03:16Z

@seanshpark @RussKeldorph LEGACY_BACKEND x86 also has no support for Linux currently. Or, I could say, RyuJIT/x86 and LEGACY_BACKEND should be equivalent in terms of Linux support, as far as I know. Thus, I don't believe we should ever enable LEGACY_BACKEND for Linux x86. We should only enable RyuJIT.

BruceForstall · 2016-11-23T16:07:26Z

@JosephTremoulet @RussKeldorph The only time you might want to use COMPLUS_AltJit is to use the LEGACY_BACKEND x86 JIT when you've also built JIT32 (via buildJit32). Pretty rare case. (In this case you'd use COMPlus_AltJitName=legacyjit.dll).

BruceForstall · 2016-11-23T16:08:35Z

@adiaaida I haven't touched any of the lab jobs. What do I need to do?

michellemcdaniel · 2016-11-23T16:12:42Z

I'm working on a patch for you. There aren't many changes. The one thing I'm not sure about is how to specify that we want to use legacyjit instead of the regular jit.

BruceForstall · 2016-11-23T16:30:23Z

@adiaaida @RussKeldorph We should consider whether we want to keep any of the legacy x86 lab runs or jobs. I don't think we need them. Also, we should get rid of all the "ryujit" x86 jobs.

BruceForstall · 2016-11-23T16:32:42Z

@adiaaida @RussKeldorph Ok, I take that back. We should have jobs that exercise the fallback-to-JIT32 path using COMPlus_UseWindowsX86CoreLegacyJit=1. Those could replace the "legacy x86" jobs (i.e., the "legacy x86" jobs could change to set this variable).

BruceForstall · 2016-11-23T16:40:57Z

Another option that might be simpler is to always build legacyjit.dll as an altjit. Then, build it additionally as a non-altjit compatjit.dll in the case where buildjit32 is not specified. Thus, we would always have the same set of jit dll names built, and the content of compatjit.dll would either be jit32 or LEGACY_BACKEND RyuJIT depending on whether buildjit32 was specified.

I'm going to go with this, to simplify thinking about the build.

BruceForstall · 2016-11-23T20:12:56Z

@adiaaida @jashook @RussKeldorph @mmitche I made some big changes to netci.groovy to: (1) remove x86ryujit -- it is now just x86, (2) add x86compatjit. Comments?

BruceForstall · 2016-11-23T21:05:31Z

Ok, now I'm stuck. The PR generator hits this:

20:49:50 Processing DSL script netci.groovy
20:50:01 FATAL: assert false
20:50:01 
20:50:01 Assertion failed: 
20:50:01 
20:50:01 assert false
20:50:01 
20:50:01 
20:50:01 	at org.codehaus.groovy.runtime.InvokerHelper.assertFailed(InvokerHelper.java:386)
20:50:01 	at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.assertFailed(ScriptBytecodeAdapter.java:658)
20:50:01 	at netci.addTriggers(netci.groovy:1410)
20:50:01 	at netci$addTriggers.callStatic(Unknown Source)
20:50:01 	at netci$_run_closure1_closure10_closure11_closure12_closure13.doCall(netci.groovy:2007)
20:50:01 	at sun.reflect.GeneratedMethodAccessor22960.invoke(Unknown Source)

That's this:

             default:
                 println("Unknown scenario: ${os} ${architecture} ${scenario}");
                 assert false
                 break

but I don't see the println output, so I don't know how to debug this.

Suggestions?

RussKeldorph · 2016-11-23T21:31:13Z

netci.groovy

-               'gcstress0x3' : ['COMPlus_GCStress'  : '0x3'], 'gcstress0xc' : ['COMPlus_GCStress'  : '0xC'],
+               'gcstress0x3' : ['COMPlus_GCStress'  : '0x3'],
+               'gcstress0xc' : ['COMPlus_GCStress'  : '0xC'],
               'zapdisable' : ['COMPlus_ZapDisable'  : '0xC'],


Might as well fix the 0xC typo while you're here.

RussKeldorph · 2016-11-23T21:32:17Z

netci.groovy

-               'jitstressregs3' : ['COMPlus_JitStressRegs' : '3'], 'jitstressregs4' : ['COMPlus_JitStressRegs' : '4'],
-               'jitstressregs8' : ['COMPlus_JitStressRegs' : '8'], 'jitstressregs0x10' : ['COMPlus_JitStressRegs' : '0x10'],
+    def static jitStressModeScenarios = [
+               'minopts' : ['COMPlus_JITMinOpts' : '1'],


Funny I had this same reformatting in my pending change. Oh well...probably not too hard to merge.

RussKeldorph · 2016-11-23T21:34:42Z

netci.groovy

    def osGroup = getOSGroup(os)
    switch (architecture) {
-        case 'x64':
+        case 'x64': // editor brace matching: {


What is this for? To help you navigate the unreasonably large blocks?

RussKeldorph · 2016-11-23T21:45:17Z

but I don't see the println output, so I don't know how to debug this.

@mmitche How do we printf-debug the groovy?

JIT32 becomes compatjit.dll and RyuJIT LEGACY_BACKEND becomes legacyjit.dll (and is an altjit). If JIT32 is not being built, then RyuJIT LEGACY_BACKEND becomes compatjit.dll and is a normal jit (not an altjit). Both clrjit.dll and compatjit.dll are added to the JIT NuGet package.

BruceForstall · 2016-11-23T22:33:18Z

@dotnet-bot test ci please

michellemcdaniel · 2016-11-23T22:38:42Z

LGTM!

pgavlin · 2016-11-23T22:41:09Z

CMake changes look good to me,

jashook · 2016-11-23T23:20:41Z

I believe printing is just:

print "hello world"

BruceForstall · 2016-11-23T23:22:49Z

@swaroop-sridhar Is this just a random error (in the Windows_NT arm Cross Debug Build task)?

22:52:05 BUILDTEST: Commencing CoreCLR repo test build
22:52:05 BUILDTEST: Checking prerequisites
22:52:08 Tools are already initialized.
22:52:08 JSON file: C:\Users\DOTNET~1\AppData\Local\Temp\coreclr_gcstress_9236\project.json
22:52:08 {     "dependencies": {     "runtime.win7-arm.Microsoft.NETCore.CoreDisTools": "1.0.1-prerelease-*"     },     "frameworks": { "dnxcore50": { } }     } 
22:52:08 Downloading CoreDisTools package
22:52:08 "D:\j\workspace\arm_cross_deb---74b8368e\tests\..\Tools\dotnetcli\dotnet.exe " restore "C:\Users\DOTNET~1\AppData\Local\Temp\coreclr_gcstress_9236\project.json" --source https://dotnet.myget.org/F/dotnet-core/ --packages "D:\j\workspace\arm_cross_deb---74b8368e\tests\..\Packages"
22:52:08 log  : Restoring packages for C:\Users\dotnet-bot\AppData\Local\Temp\coreclr_gcstress_9236\project.json...
22:52:09 
error: Unable to resolve 'runtime.win7-arm.Microsoft.NETCore.CoreDisTools (>= 1.0.1-prerelease)' for 'DNXCore,Version=v5.0'.
22:52:09 log  : Writing lock file to disk. Path: C:\Users\dotnet-bot\AppData\Local\Temp\coreclr_gcstress_9236\project.lock.json
22:52:09 log  : C:\Users\DOTNET~1\AppData\Local\Temp\coreclr_gcstress_9236\project.json
22:52:09 log  : Restore failed in 827ms.
22:52:09 
22:52:09 Errors in C:\Users\DOTNET~1\AppData\Local\Temp\coreclr_gcstress_9236\project.json
22:52:09     Unable to resolve 'runtime.win7-arm.Microsoft.NETCore.CoreDisTools (>= 1.0.1-prerelease)' for 'DNXCore,Version=v5.0'.

I'm going to try it again.

BruceForstall · 2016-11-23T23:23:00Z

@dotnet-bot test Windows_NT arm Cross Debug Build

jashook

Looks good minus the open question about whether to run these as PR triggers and confusion about compatjit.dll.

jashook · 2016-11-23T23:38:44Z

src/jit/compatjit/CMakeLists.txt

@@ -0,0 +1,66 @@
+project(compatjit)
+
+# This compatjit.dll is only built if we are not building JIT32 as compatjit.dll.


Does this in the open build legacy_backend twice? Giving two dlls, compatjit.dll and legacyjit.dll which are the same thing?

Yes. They're not quite the same, since one is an altjit and one is not. But essentially they are the same.

jashook · 2016-11-23T23:43:53Z

netci.groovy

                    }
                    // For x86, only add per-commit jobs for Windows
-                    else if (architecture == 'x86ryujit' || architecture == 'x86lb') {
+                    else if (architecture == 'x86' || architecture == 'x86compatjit' || architecture == 'x86lb') {


Do we need to run x86 compatjit as a PR trigger? Seems like this and legacy backend can now be outerloop tests run either per checkin or on a timer.

I agree. We should back this off if these are for triggers. In this case, I just replaced the 2 existing with the 3 new archs. I'll call this a to-do.

swaroop-sridhar · 2016-11-23T23:49:42Z

@BruceForstall The failure downloading CoreDistools doesn't look like a random failure.
I think some change to the test infrastructure is causing CoreDistools for ARM to be downloaded -- and there's no CoreDisTools for ARM/ARM64.

BruceForstall · 2016-11-24T00:13:54Z

@swaroop-sridhar well, when I retested, it worked :-)

swaroop-sridhar · 2016-11-24T00:16:29Z

In the log for the new run, we can see that the error still exists, but doesn't cause the job to fail.
Errors in C:\Users\DOTNET~1\AppData\Local\Temp\coreclr_gcstress_18931\project.json Unable to resolve 'runtime.win7-arm.Microsoft.NETCore.CoreDisTools (>= 1.0.1-prerelease)' for 'DNXCore,Version=v5.0'.
So, like this problem is likely around but unnoticed because it doesn't affect anything.
So, the original failure is something else - unrelated to CoreDisTools.

jamesqo · 2016-11-24T04:54:39Z

Nice! 🎉

…RyuJit Make RyuJIT/x86 the default x86 JIT Commit migrated from dotnet/coreclr@db767b8

BruceForstall added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 23, 2016

dnfclas added the cla-already-signed label Nov 23, 2016

BruceForstall force-pushed the SwitchX86ToRyuJit branch from 4357682 to 57809d5 Compare November 23, 2016 07:07

ericstj approved these changes Nov 23, 2016

View reviewed changes

BruceForstall force-pushed the SwitchX86ToRyuJit branch from 57809d5 to fae4074 Compare November 23, 2016 20:09

RussKeldorph approved these changes Nov 23, 2016

View reviewed changes

BruceForstall force-pushed the SwitchX86ToRyuJit branch from 067bfde to aad8402 Compare November 23, 2016 22:33

RussKeldorph approved these changes Nov 23, 2016

View reviewed changes

jashook reviewed Nov 23, 2016

View reviewed changes

BruceForstall merged commit db767b8 into dotnet:master Nov 24, 2016

BruceForstall deleted the SwitchX86ToRyuJit branch November 24, 2016 00:24

richlander mentioned this pull request Apr 21, 2017

RyuJIT is the x86 JIT in .NET Core 2.0 – Replacing JIT32 dotnet/announcements#10

Open

karelz modified the milestone: 2.0.0 Aug 28, 2017

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#8260 from BruceForstall/SwitchX86To…

605c5fb

…RyuJit Make RyuJIT/x86 the default x86 JIT Commit migrated from dotnet/coreclr@db767b8

		@@ -0,0 +1,66 @@
		project(compatjit)

		# This compatjit.dll is only built if we are not building JIT32 as compatjit.dll.

Conversation

BruceForstall commented Nov 23, 2016

Uh oh!

BruceForstall commented Nov 23, 2016

Uh oh!

seanshpark commented Nov 23, 2016

Uh oh!

JosephTremoulet commented Nov 23, 2016

Uh oh!

RussKeldorph commented Nov 23, 2016

Uh oh!

RussKeldorph commented Nov 23, 2016

Uh oh!

michellemcdaniel commented Nov 23, 2016

Uh oh!

BruceForstall commented Nov 23, 2016

Uh oh!

BruceForstall commented Nov 23, 2016

Uh oh!

BruceForstall commented Nov 23, 2016

Uh oh!

michellemcdaniel commented Nov 23, 2016

Uh oh!

BruceForstall commented Nov 23, 2016

Uh oh!

BruceForstall commented Nov 23, 2016

Uh oh!

BruceForstall commented Nov 23, 2016

Uh oh!

BruceForstall commented Nov 23, 2016

Uh oh!

BruceForstall commented Nov 23, 2016

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!

RussKeldorph commented Nov 23, 2016

Uh oh!

BruceForstall commented Nov 23, 2016

Uh oh!

michellemcdaniel commented Nov 23, 2016

Uh oh!

pgavlin commented Nov 23, 2016

Uh oh!

jashook commented Nov 23, 2016

Uh oh!

BruceForstall commented Nov 23, 2016

Uh oh!

BruceForstall commented Nov 23, 2016

Uh oh!

jashook left a comment • 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!

swaroop-sridhar commented Nov 23, 2016

Uh oh!

BruceForstall commented Nov 24, 2016

Uh oh!

swaroop-sridhar commented Nov 24, 2016

Uh oh!

jamesqo commented Nov 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jashook left a comment •

edited

Loading