Remove support for the x86 compat JIT from .NET Core. #11262

pgavlin · 2017-04-27T18:35:17Z

These changes remove support for the x86 compat JIT from the build, the
runtime, and the various perf/test scripts.

Fixes #10733.

michellemcdaniel · 2017-04-27T18:42:43Z

perf.groovy

@@ -27,16 +27,11 @@ def static getOSGroup(def os) {
 // Setup perflab tests runs
 [true, false].each { isPR ->
    ['Windows_NT'].each { os ->
-        ['x64', 'x86', 'x86jit32'].each { arch ->
+        ['x64', 'x86' ].each { arch ->


Remove the space

michellemcdaniel · 2017-04-27T18:43:49Z

perf.groovy

-                testEnv = '-testEnv %WORKSPACE%\\tests\\x86\\compatjit_x86_testenv.cmd'
-            }
-            else if (arch == 'x86')
+            if (arch == 'x86')


Do we need to keep this? This was put in on the off chance that we switched back to jit32 being the main jit.

I don't think we need to keep this given the current PoR re: jit32.

Might as well take it out and delete the testenv file then.

michellemcdaniel · 2017-04-27T18:44:04Z

perf.groovy

@@ -126,14 +114,9 @@ def static getOSGroup(def os) {
 // Setup throughput perflab tests runs
 [true, false].each { isPR ->
    ['Windows_NT'].each { os ->
-        ['x64', 'x86', 'x86jit32'].each { arch ->
+        ['x64', 'x86' ].each { arch ->


Remove the space

michellemcdaniel · 2017-04-27T18:46:07Z

@RussKeldorph suggested we still wanted to have throughput testing for jit32. Is that no longer true?

wtgodbe

Pipeline changes LGTM. This should resolve https://github.com/dotnet/coreclr/issues/10734 as well.

wtgodbe · 2017-04-27T19:00:55Z

Once this is merged, I'll file an issue against myself to combine the Windows & Windows x86 jobs into one single job that we can use for either arch.

BruceForstall · 2017-04-27T19:45:48Z

src/CMakeLists.txt

@@ -150,10 +150,6 @@ add_subdirectory(gcinfo)
 add_subdirectory(coreclr)
 add_subdirectory(jit)

-if(IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/jit32")
-  add_subdirectory(jit32)


nit: no need to leave the extra blank line here

BruceForstall · 2017-04-27T19:48:16Z

src/vm/codeman.cpp

@@ -1660,25 +1660,8 @@ BOOL EEJitManager::LoadJIT()
    if ((!IsCompilationProcess() || !fUseRyuJit) &&     // Use RyuJIT for all NGEN, unless we're falling back to JIT64 for everything.
        (newJitCompiler != nullptr))    // the main JIT must successfully load before we try loading the fallback JIT
    {
-        BOOL fUsingCompatJit = FALSE;
-
        if (!fUseRyuJit)


Remove comment above:

// // For .NET Core 1.2, RyuJIT for x86 is the primary jit (clrjit.dll) and JIT32 for x86 is the fallback, legacy JIT (compatjit.dll). // Thus, the COMPlus_useLegacyJit=1 mechanism has been enabled for x86 CoreCLR. This scenario does not have the UseRyuJIT // registry key, nor the AppX binder mode.

Actually, the entire #ifdef starting with:

#if defined(_TARGET_X86_) // If COMPlus_UseLegacyJit=1, then we fall back to compatjit.dll. // // This fallback mechanism was introduced for Visual Studio "14" Preview, when JIT64 (the legacy JIT) was replaced with

should be deleted, since there is no remaining fallback.

In reply to: 113786338 [](ancestors = 113786338)

Ah, yes--this is VM code, so we won't mirror the changes back. Good suggestion.

BruceForstall · 2017-04-27T19:54:55Z

tests/x86/legacyjit_x86_testenv.cmd

-@REM set COMPLUS_JitFuncInfoLogFile=%__FuncInfoDir%\FuncInfo.txt
-@REM ==== Uncomment above
-
-@REM -------------------------------------------------------------------------


This file is useful to keep. But maybe we can move it to the tests directory and name it altjit_testenv.cmd. I've used it recently for an arm32 altjit test run, for example.

Would you prefer that to just maintaining your own local testenv script? Even if we left this around as altjit_testenv.cmd you'd need to edit it first s.t. COMPlus_AltJitName was correct, right?

We could change it to protononjit.dll.

I can maintain my own script, of course, but it seems like there is value here for other people, to share.

In reply to: 113788464 [](ancestors = 113788464)

protononjit.dll sounds applicable enough to me.

Note that I suggested altjit_testenv.cmd because you can use it for any altjit. protononjit.dll is one case (and a good default). linuxnonjit.dll is another.

In reply to: 113814905 [](ancestors = 113814905)

michellemcdaniel · 2017-04-27T20:05:00Z

perf.groovy

-                        batchFile("C:\\Tools\\nuget.exe install runtime.win7-${architecture}.Microsoft.NETCore.Jit -Source https://dotnet.myget.org/F/dotnet-core -OutputDirectory \"%WORKSPACE%\" -Prerelease -ExcludeVersion\n" +
-                        "xcopy \"%WORKSPACE%\\runtime.win7-x86.Microsoft.NETCore.Jit\\runtimes\\win7-x86\\native\\compatjit.dll\" \"%WORKSPACE%\\bin\\Product\\${os}.${architecture}.${configuration}\" /Y")
-                    }
-
                    batchFile("tests\\runtest.cmd ${configuration} ${architecture} GenerateLayoutOnly")

                    batchFile("tests\\scripts\\run-xunit-perf.cmd -arch ${arch} -configuration ${configuration} ${testEnv} -testBinLoc bin\\tests\\${os}.${architecture}.${configuration}\\performance\\perflab\\Perflab -library -uploadToBenchview \"%WORKSPACE%\\Microsoft.Benchview.JSONFormat\\tools\" -runtype ${runType}")


We can probably just remove the ${testEnv} vars in these lines. But I think it's good to keep the -testEnv option in run-xunit-perf.cmd in case people want to run perf on scenarios where they want to use a testenv script

michellemcdaniel · 2017-04-27T20:05:26Z

tests/scripts/run-throughput-perf.py

@@ -126,7 +125,7 @@ def validate_arg(arg, check):
        if not helper(arg):
            raise Exception('Argument: %s is not valid.' % (arg))

-    valid_archs = {'Windows_NT': ['x86', 'x64', 'x86jit32'], 'Linux': ['x64']}
+    valid_archs = {'Windows_NT': ['x86', 'x64' ], 'Linux': ['x64']}


Extra space

BruceForstall · 2017-04-27T21:39:48Z

src/inc/clrconfigvalues.h

-// UseRyuJIT is only looked up in HKLM in the registry, not in config files or environment variables. It should only be set by the .NET 4.6 (and later version) installers,
-// not by users. See the "RyuJIT Compatibility Fallback Design Specification.docx" document for details.
-RETAIL_CONFIG_DWORD_INFO_EX(INTERNAL_UseRyuJit, W("UseRyuJIT"), 0, "Set to 1 by .NET 4.6 installer to indicate RyuJIT should be used, not JIT64.", CLRConfig::IgnoreEnv | CLRConfig::IgnoreHKCU | CLRConfig::IgnoreConfigFiles)
-RETAIL_CONFIG_DWORD_INFO(EXTERNAL_UseLegacyJit, W("useLegacyJit"), 0, "Set to 1 to do all JITing with compatjit.dll. Only applicable to x64.")
 RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_DisableNativeImageLoadList, W("DisableNativeImageLoadList"), "Refuse to load native images corresponding to one of the assemblies on this semicolon-delimited list of assembly names.", CLRConfig::REGUTIL_default)


If you're going to bother touching these, you should also delete EXTERNAL_DisableNativeImageLoadList and everything related to it (and there is a fair bit, in eeconfig.*), since it's no longer used, and was only added to support JIT64 fallback.

Will do. I wasn't sure if that option had use cases outside of JIT64 fallback.

BruceForstall · 2017-04-27T21:45:01Z

src/vm/codeman.cpp

-            // to load it, that we fail noisily. We don't do that currently.
-            ICorJitCompiler* fallbackICorJitCompiler;
-            g_JitLoadData.jld_id = JIT_LOAD_LEGACY;
-            LoadAndInitializeJIT(pwzJitName, &m_JITCompilerOther, &fallbackICorJitCompiler, &g_JitLoadData);


Above, there is:

enum JIT_LOAD_JIT_ID { JIT_LOAD_MAIN = 500, // The "main" JIT. Normally, this is named "clrjit.dll". Start at a number that is somewhat uncommon (i.e., not zero or 1) to help distinguish from garbage, in process dumps. JIT_LOAD_LEGACY, // The "legacy" JIT. Normally, this is named "compatjit.dll". This applies to AMD64 on Windows desktop, or x86 on Windows .NET Core. JIT_LOAD_ALTJIT // An "altjit". By default, named "protojit.dll". Used both internally, as well as externally for JIT CTP builds. };

And now JIT_LOAD_LEGACY is unused. However, I don't really want to renumber these; I'd like the numbers to be the same for all platforms, all architectures, desktop or CoreCLR, if possible. Probably best to change it to:

enum JIT_LOAD_JIT_ID { JIT_LOAD_MAIN = 500, // The "main" JIT. Normally, this is named "clrjit.dll". Start at a number that is somewhat uncommon (i.e., not zero or 1) to help distinguish from garbage, in process dumps. // 501 is JIT_LOAD_LEGACY on some platforms; don't reuse. JIT_LOAD_ALTJIT = 502 // An "altjit". By default, named "protojit.dll". Used both internally, as well as externally for JIT CTP builds. };

Can you elaborate on your concern re: renumbering? Do we report this data somehow s.t. the renumbering could make some statistics ambiguous?

(your suggestion seems fine, I just want to understand the motivation a bit better)

It appears in Watson dumps, mainly. Yes, Watson dumps (probably) have platform, version, and other distinguishing characteristics, but it's simply easier if the numbers are "globally" unique.

Or, globally "the same"

These changes remove support for the x86 compat JIT from the build, the runtime, and the various perf/test scripts. Fixes #10733, #10734.

michellemcdaniel · 2017-04-27T23:21:47Z

@dotnet-bot test ci please

pgavlin added the area-CodeGen label Apr 27, 2017

pgavlin added this to the 2.0.0 milestone Apr 27, 2017

pgavlin requested review from michellemcdaniel, BruceForstall and wtgodbe April 27, 2017 18:35

dnfclas added the cla-already-signed label Apr 27, 2017

michellemcdaniel reviewed Apr 27, 2017

View reviewed changes

pgavlin force-pushed the NoCompatJit branch from db89a93 to fb412aa Compare April 27, 2017 18:50

wtgodbe approved these changes Apr 27, 2017

View reviewed changes

BruceForstall reviewed Apr 27, 2017

View reviewed changes

michellemcdaniel reviewed Apr 27, 2017

View reviewed changes

BruceForstall reviewed Apr 27, 2017

View reviewed changes

Remove support for the x86 compat JIT from .NET Core.

abd7672

These changes remove support for the x86 compat JIT from the build, the runtime, and the various perf/test scripts. Fixes #10733, #10734.

pgavlin force-pushed the NoCompatJit branch from 0d4919b to abd7672 Compare April 27, 2017 22:33

RussKeldorph approved these changes Apr 27, 2017

View reviewed changes

BruceForstall approved these changes Apr 27, 2017

View reviewed changes

michellemcdaniel approved these changes Apr 27, 2017

View reviewed changes

pgavlin merged commit dd4d0da into dotnet:master May 1, 2017

pgavlin deleted the NoCompatJit branch September 11, 2017 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove support for the x86 compat JIT from .NET Core. #11262

Remove support for the x86 compat JIT from .NET Core. #11262

pgavlin commented Apr 27, 2017

michellemcdaniel Apr 27, 2017

michellemcdaniel Apr 27, 2017

pgavlin Apr 27, 2017

michellemcdaniel Apr 27, 2017

michellemcdaniel Apr 27, 2017

michellemcdaniel commented Apr 27, 2017

wtgodbe left a comment

wtgodbe commented Apr 27, 2017

BruceForstall Apr 27, 2017

BruceForstall Apr 27, 2017

BruceForstall Apr 27, 2017

pgavlin Apr 27, 2017

BruceForstall Apr 27, 2017

pgavlin Apr 27, 2017

BruceForstall Apr 27, 2017

pgavlin Apr 27, 2017

BruceForstall Apr 27, 2017

michellemcdaniel Apr 27, 2017

michellemcdaniel Apr 27, 2017

BruceForstall Apr 27, 2017

pgavlin Apr 27, 2017

BruceForstall Apr 27, 2017

pgavlin Apr 27, 2017

pgavlin Apr 27, 2017

BruceForstall Apr 27, 2017

BruceForstall Apr 27, 2017

michellemcdaniel commented Apr 27, 2017

Remove support for the x86 compat JIT from .NET Core. #11262

Remove support for the x86 compat JIT from .NET Core. #11262

Conversation

pgavlin commented Apr 27, 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

michellemcdaniel commented Apr 27, 2017

wtgodbe left a comment

Choose a reason for hiding this comment

wtgodbe commented Apr 27, 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

Choose a reason for hiding this comment

michellemcdaniel commented Apr 27, 2017