Universal arm crossgen compiler #58279

davidwrighton · 2021-08-27T17:55:16Z

Stop generating OS specific compilers for the arm and arm64 architectures for use in crossgen2

As it turns out the OS differences here are relatively minor and fairly easy to handle with runtime switches.
However, when an exact target is known we can generate better code by having the various if statements evaluate to a constant true/false. (This is done for all runtime jit builds)
This has been validated to produce binary identical output for generating System.Private.CoreLib.dll on the various architectures.
This work reduces the distribution size of crossgen2, and also reduces the amount of time it takes to build everything (Most notably, the Mac build times in PR's are reduced by 5-7 minutes, but improvements are visible across all of/architecture pairs)

- Except for ABI specific details... - Notably the FEATURE_ARG_SPLIT for ARM32 and the OSX ARM64 abi differences

…wordy

ghost · 2021-08-27T17:55:24Z

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

Stop generating OS specific compilers for the arm and arm64 architectures for use in crossgen2

As it turns out the OS differences here are relatively minor and fairly easy to handle with runtime switches.
However, when an exact target is known we can generate better code by having the various if statements evaluate to a constant true/false. (This is done for all runtime jit builds)
This work is in progress, as I haven't yet verified that we generate binary identical output with the new universal compilers, but I'm interested in feedback, and finding out how this affects build times in CI

Author:	davidwrighton
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

- Don't special case Windows handling of clrjit.dll, just use the crossgen2 targetted one - Fix cross compilation targets - Build the universal cross clrjit targets, but they don't need to be part of the runtimepack - Fixup setting of compMatchedVM for macOS X64 in crossgen2

- Adjust superpmi setup to know about the new dll names - Adjust DEBUG_ARG_SLOTS handling to work with osx arm64 abi. (By disabling anything meaningful)

davidwrighton · 2021-09-01T22:02:09Z

I don't intend to fix the formatting until all other comments are complete, as formatting fixes have a tendency of making merges a bit harder. I'd like to get a round of feedback dealt with before that.

davidwrighton · 2021-09-01T22:03:47Z

@dotnet/jit-contrib I'd like to get feedback for this work. It works nicely, and the improvements to PR times, and crossgen2 file size are compelling, but I'd like some guidance on naming and generally ifdef hygiene in my changes.

kasperk81 · 2021-09-04T13:39:01Z

src/coreclr/jit/CMakeLists.txt

  create_standalone_jit(TARGET clrjit_unix_x64_${ARCH_HOST_NAME} OS unix ARCH x64 DESTINATIONS .)
-  create_standalone_jit(TARGET clrjit_win_arm64_${ARCH_HOST_NAME} OS win ARCH arm64 DESTINATIONS .)
  create_standalone_jit(TARGET clrjit_win_x64_${ARCH_HOST_NAME} OS win ARCH x64 DESTINATIONS .)


are "universal" x64 and x86 also possible?

In theory this is possible, but its quite a bit more difficult and would require a fair amount more changes. The Arm64 and Arm architecture jits produce very similar code, but the x64 ABI is quite different between Unix platforms and Windows (for instance, there are quite a lot of fields on GenTree structures that are different for Unix and Windows.

AndyAyersMS

Since the various TARGET_ OS defines still exist it seems like might be easy for someone to do the wrong thing and use them when they shouldn't.

I wonder if we could make this more obvious at build time somehow?

AndyAyersMS · 2021-09-12T16:20:17Z

src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.cpp

-#ifdef TARGET_UNIX
+#ifdef TARGET_OSX
+        pEEInfoOut->osType                                     = CORINFO_MACOS;
+#elif defined(TARGET_UNIX)


I wonder why we bother with faking up EEInfo instead of blowing things up.

@BruceForstall any idea?

Maybe the JIT (or JIT32/JIT64) only called getEEInfo() once and reused the info for all method compiles, so only some MCs would have it? Looks like RyuJIT calls it for every compile.

AndyAyersMS · 2021-09-12T16:36:27Z

src/coreclr/jit/target.h

+#define compFeatureVarArg() (TargetOS::IsWindows && !TargetArchitecture::IsArm32)
+#define compMacOsArm64Abi() (TargetArchitecture::IsArm64 && TargetOS::IsMacOS)
+#define compFeatureArgSplit() ( TargetArchitecture::IsArm32 || (TargetOS::IsWindows && TargetArchitecture::IsArm64) )
+


Seems odd to have #defines that look like methods; why not just create methods?

Turned them into methods. Seems to work fine.

…rossgen_compiler

- Undef all TARGET_ OS macros within the JIT so that they can't be used there - Convert the feature macros to inline functions. Should have equivalent perf on release/chk builds as macro approach, and is less problematic in general.

davidwrighton · 2021-09-15T15:35:51Z

@AndyAyersMS I've set it up to undef the TARGET_ macros for the OS and it seems to not have broken anything.

AndyAyersMS

Thanks. Changes LGTM (modulo formatting).

I'd like @BruceForstall to also take a look.

BruceForstall

In general, it looks pretty good, and not too much overhead, either conceptual or runtime.

Misc:

It looks like superpmi-replay.py needs some change to handle the "universal" naming.
Will it work to use superpmi replay with an altjit? E.g., use a Windows arm64 collection with a Linux arm64 altjit? It seems like the collection is going to force the "universal" binary to generate for windows, not Linux.
Looks like the overhead might not be too bad, although we will include CFI support for all builds, even though it's only needed for Linux targeting CoreRT.
There are now more fields that are always available that can be set/read even when illegal. We'll have to think about ways to verify that doesn't happen, maybe new asserts or the like. Like accessing the CFI data array. It's a case of converting compile-time to run-time error.

BruceForstall · 2021-09-16T00:14:04Z

src/coreclr/ToolBox/superpmi/superpmi-shared/methodcontext.cpp

-#ifdef TARGET_UNIX
+#ifdef TARGET_OSX
+        pEEInfoOut->osType                                     = CORINFO_MACOS;
+#elif defined(TARGET_UNIX)


Maybe the JIT (or JIT32/JIT64) only called getEEInfo() once and reused the info for all method compiles, so only some MCs would have it? Looks like RyuJIT calls it for every compile.

BruceForstall · 2021-09-16T00:14:57Z

src/coreclr/jit/jit.h

+// On all platforms except Arm64 OSX arguments on the stack are taking
+// register size slots. On these platforms we could check that stack slots count
+// matches our new byte size calculations.
+#define DEBUG_ARG_SLOTS_ASSERT(x) assert(compMacOsArm64Abi() || x)


Suggested change

#define DEBUG_ARG_SLOTS_ASSERT(x) assert(compMacOsArm64Abi() || x)

#define DEBUG_ARG_SLOTS_ASSERT(x) assert(compMacOsArm64Abi() || (x))

Extra parens around x, for precedence safety.

BruceForstall · 2021-09-16T01:01:29Z

src/coreclr/jit/codegenarmarch.cpp

@@ -2307,8 +2313,8 @@ void CodeGen::genCallInstruction(GenTreeCall* call)
 #endif // TARGET_ARM
            }
        }
-#if FEATURE_ARG_SPLIT
-        else if (curArgTabEntry->IsSplit())
+#if FEATURE_ARG_SPLIT_SUPPORTED


Some minor comments about FEATURE_ARG_SPLIT_SUPPORTED: it's not clear the renaming of the ifdef matters. Also, now that it is always enabled for arm32 & arm64, we could even remove the ifdefs in arm32/arm64-only code files, like codegenarmarch.cpp. Also, here there's no need to check for compFeatureArgSplit() since IsSplit() is also doing that.

BruceForstall · 2021-09-16T01:07:20Z

src/coreclr/jit/codegenlinear.cpp

-#if FEATURE_ARG_SPLIT
-            else if (tree->OperIsPutArgSplit())
+#if FEATURE_ARG_SPLIT_SUPPORTED
+            else if (compFeatureArgSplit() && tree->OperIsPutArgSplit())


Presumably, in cases like this we don't need to check for compFeatureArgSplit() && because we would never have inserted a GT_PUTARG_SPLIT tree node unless it was legal?

BruceForstall · 2021-09-16T01:11:23Z

src/coreclr/jit/codegenxarch.cpp

-#elif defined(TARGET_WINDOWS) && defined(TARGET_AMD64)
-    assert(!"Multireg store to SIMD reg not supported on Windows x64");
+#elif defined(TARGET_AMD64)
+    assert(!TargetOS::IsWindows && !"Multireg store to SIMD reg not supported on Windows x64");


Suggested change

assert(!TargetOS::IsWindows && !"Multireg store to SIMD reg not supported on Windows x64");

assert(!TargetOS::IsWindows || !"Multireg store to SIMD reg not supported on Windows x64");

Shouldn't this use ||?

Yes. Oops. Thanks for catching that.

BruceForstall · 2021-09-16T01:15:54Z

src/coreclr/jit/compiler.cpp

+#ifdef TARGET_OS_RUNTIMEDETERMINED
+    if (!TargetOS::OSSettingConfigured)
+    {
+        CORINFO_EE_INFO* eeInfo       = eeGetEEInfo();


Don't need this line; it was just queried 5 lines above.

BruceForstall · 2021-09-16T01:22:29Z

src/coreclr/jit/compiler.cpp

+        TargetOS::IsMacOS             = eeInfo->osType == CORINFO_MACOS;
+        TargetOS::IsUnix              = (eeInfo->osType == CORINFO_UNIX) || (eeInfo->osType == CORINFO_MACOS);
+        TargetOS::IsWindows           = eeInfo->osType == CORINFO_WINNT;
+        TargetOS::OSSettingConfigured = true;


Is it safe for these to be set as statics? E.g., multi-thread safe?

Do we want to support changing the target on multiple subsequent JIT invocations? E.g., would we want to support merging SuperPMI method contexts for different target OS settings and then replaying them all one after another with the same loaded JIT? Having these be static seems dangerous.

I wonder if they should be on the JIT "host" api ICorJitHost.

BruceForstall · 2021-09-16T01:26:10Z

src/coreclr/jit/gentree.h

-        return gtOper == GT_PUTARG_SPLIT;
-#else // !FEATURE_ARG_SPLIT
+#if FEATURE_ARG_SPLIT_SUPPORTED
+        return compFeatureArgSplit() && (gtOper == GT_PUTARG_SPLIT);


This is another case where it seems like we shouldn't have to check compFeatureArgSplit; we should just ensure we don't create a GT_PUTARG_SPLIT if it's not legal.

davidwrighton · 2021-09-16T18:43:22Z

@BruceForstall

Ok. I'll make a change, but I don't know how to use that script
Possibly. SuperPMI would need to specify that it is attempting to compile for the target architecture. Something would likely need to change in SuperPMI to make that happen, but I'm not familiar enough with how to use it to feel comfortable trying to make that change.
Yeah, I'm pretty sure the overhead is minimal.
Yes. This is a major concern of mine. Looking at what I needed to do, the number of variables newly exposed in this current change is pretty low, and thus low-risk, but if we want to ever try to merge the X64 jits, it will be highly risky as there are many fields that would come into use.

BruceForstall · 2021-09-16T19:17:39Z

Ok. I'll make a change, but I don't know how to use that script

@kunalspathak wrote the superpmi-replay.py script, so he can probably advise.

Re (2): we might need some way for superpmi to override the target OS that is baked into the collection. I guess I wouldn't worry about it for now, as we do have arch/OS matching collections, which most people would use.

davidwrighton · 2021-09-16T23:31:45Z

@BruceForstall, your comment on initialization made me realize that it was somewhat broken in some multithreading scenarios on Crossgen2, so I plumbed through the Jit a way to set it explicitly, which looks much nicer. I also jammed the OS into superpmi, but it isn't all that pretty. My guess is that superpmi probably works with this change now.

kunalspathak · 2021-09-17T04:58:37Z

@kunalspathak wrote the superpmi-replay.py script, so he can probably advise.

Changes in superpmi-replay.py looks good. Some minor changes in other scripts:

diff --git a/src/coreclr/scripts/jitrollingbuild.py b/src/coreclr/scripts/jitrollingbuild.py
index c5bf6d065b5..0003805b499 100644
--- a/src/coreclr/scripts/jitrollingbuild.py
+++ b/src/coreclr/scripts/jitrollingbuild.py
@@ -309,8 +309,8 @@ def upload_command(coreclr_args):

     # Next, look for any and all cross-compilation JITs. These are named, e.g.:
     #   clrjit_unix_x64_x64.dll
-    #   clrjit_win_arm_x64.dll
-    #   clrjit_win_arm64_x64.dll
+    #   clrjit_universal_arm_x64.dll
+    #   clrjit_universal_arm64_x64.dll
     # and so on, and live in the same product directory as the primary JIT.
     #
     # Note that the expression below explicitly filters out the primary JIT since we added that above.
diff --git a/src/coreclr/scripts/superpmi.py b/src/coreclr/scripts/superpmi.py
index c43592897e7..dd69e1cd7c7 100755
--- a/src/coreclr/scripts/superpmi.py
+++ b/src/coreclr/scripts/superpmi.py
@@ -254,7 +254,7 @@ superpmi_common_parser.add_argument("--break_on_error", action="store_true", hel
 superpmi_common_parser.add_argument("--skip_cleanup", action="store_true", help=skip_cleanup_help)
 superpmi_common_parser.add_argument("--sequential", action="store_true", help="Run SuperPMI in sequential mode. Default is to run in parallel for faster runs.")
 superpmi_common_parser.add_argument("-spmi_log_file", help=spmi_log_file_help)
-superpmi_common_parser.add_argument("-jit_name", help="Specify the filename of the jit to use, e.g., 'clrjit_win_arm64_x64.dll'. Default is clrjit.dll/libclrjit.so")
+superpmi_common_parser.add_argument("-jit_name", help="Specify the filename of the jit to use, e.g., 'clrjit_universal_arm64_x64.dll'. Default is clrjit.dll/libclrjit.so")
 superpmi_common_parser.add_argument("--altjit", action="store_true", help="Set the altjit variables on replay.")
 superpmi_common_parser.add_argument("-error_limit", help=error_limit_help)

diff --git a/src/coreclr/scripts/superpmi_replay_setup.py b/src/coreclr/scripts/superpmi_replay_setup.py
index 34cc8301fc5..c6c6fcb507f 100644
--- a/src/coreclr/scripts/superpmi_replay_setup.py
+++ b/src/coreclr/scripts/superpmi_replay_setup.py
@@ -94,7 +94,7 @@ def match_correlation_files(full_path):
     file_name = os.path.basename(full_path)

     if file_name.startswith("clrjit_") and file_name.endswith(".dll") and file_name.find(
-        "osx") == -1 and file_name.find("armel") == -1:
+        "osx") == -1:
         return True

     if file_name == "superpmi.exe" or file_name == "mcs.exe":

BruceForstall

Looking good. Added a few more comments.

BruceForstall · 2021-09-17T20:24:38Z

src/coreclr/inc/corjit.h

+    // Some JIT's may support multiple OSs. This api provides a means to specify to the JIT what OS it should
+    // be trying to compile. This api does not produce any errors, any errors are to be generated by the
+    // the compileMethod call, which will call back into the VM to ensure bits are correctly setup.
+    virtual void setJitOs(CORINFO_OS os) = 0;


I know we use camel casing, but "Os" looks really weird. I don't like "Jit" in the function name.

Proposed name: setTargetOS

Ok. I have no strong opinions about naming.

BruceForstall · 2021-09-17T20:27:18Z

src/coreclr/jit/codegencommon.cpp

+        else if (TargetOS::IsUnix)
+        {
+            printf(" - Unix");
+        }


Should there be a TargetOS::IsMacOS case here, too? (sub-case of IsUnix?)

Ok. I was replicating the old behavior, but adding a new case for the MacOS case makes sense. Consider it done.

BruceForstall · 2021-09-17T20:31:34Z

src/coreclr/jit/ee_il_dll.cpp

+bool TargetOS::IsMacOS             = false;
+#endif
+
+void CILJit::setJitOs(CORINFO_OS os)


Please add a standard format JIT header comment

BruceForstall · 2021-09-17T20:35:11Z

src/coreclr/jit/jit.h

@@ -6,6 +6,8 @@
 #define _JIT_H_
 /*****************************************************************************/

+#include "targetosarch.h"


Seems like this should be moved down after #include "corjit.h"

BruceForstall · 2021-09-17T20:40:47Z

src/coreclr/jit/ee_il_dll.cpp

@@ -300,6 +300,23 @@ void CILJit::getVersionIdentifier(GUID* versionIdentifier)
    memcpy(versionIdentifier, &JITEEVersionIdentifier, sizeof(GUID));
 }

+#ifdef TARGET_OS_RUNTIMEDETERMINED
+bool TargetOS::OSSettingConfigured = false;


Should these default to the TARGET_* defines (before those get #undefed)? As is, it appears that all invokers of the JIT MUST call the new setJitOs API before calling compileMethod. If you want that behavior, then there should be a comment in ICorJitCompiler explicitly stating this ordering requirement.

I'd prefer to just make setTargetOS a required call before jitting anything via the comment approach. Making stuff happen before the #undef is likely to be pretty ugly, and the jit doesn't need any more macro goo than it already has.

BruceForstall · 2021-09-17T20:47:33Z

src/coreclr/ToolBox/superpmi/superpmi-shared/icorjitcompilerimpl.h

@@ -47,4 +47,6 @@ void getVersionIdentifier(GUID* versionIdentifier /* OUT */
 // intrinsics, so the EE should use the default size (i.e. the size of the IL implementation).
 unsigned getMaxIntrinsicSIMDVectorLength(CORJIT_FLAGS cpuCompileFlags); /* { return 0; } */

+// Used to specify which OS a JIT should be compiled for. Currently expected to only be called once pre process


nit: this should be exactly the same comment as in corjit.h

BruceForstall · 2021-09-17T21:06:18Z

src/coreclr/ToolBox/superpmi/superpmi-shim-collector/icorjitcompiler.cpp

@@ -10,6 +10,14 @@

 #define fatMC                               // this is nice to have on so ildump works...

+CORINFO_OS g_currentOs =     CORINFO_WINNT;


You should probably add this as a member to interceptor_ICJC, not a global.

And the default should probably be based on the TARGET_* defines, not always Windows. (This would be the same function as getClrVmOs?)

…rossgen_compiler

davidwrighton added 7 commits August 27, 2021 10:42

Move most OS specific handling in JIT to flag checks

97125ca

- Except for ABI specific details... - Notably the FEATURE_ARG_SPLIT for ARM32 and the OSX ARM64 abi differences

Implement arg split as flag

ac4a3e1

Move OSX Arm64 abi to flags

2ba2162

All jits build in new configuration

77824a3

Builds full clr

c3b175a

Rework checks to be more efficient on debug builds and slightly less …

bd9a51c

…wordy

add compFeatureArgSplit checks

3f4a439

davidwrighton requested a review from MichalStrehovsky as a code owner August 27, 2021 17:55

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 27, 2021

davidwrighton marked this pull request as draft August 27, 2021 18:06

EgorBo mentioned this pull request Aug 28, 2021

ARM32: Runtime crash with interfaces #58293

Closed

davidwrighton added 2 commits August 29, 2021 13:20

- Fix Windows X86 cross target build

adfc7b3

- Adjust superpmi setup to know about the new dll names - Adjust DEBUG_ARG_SLOTS handling to work with osx arm64 abi. (By disabling anything meaningful)

davidwrighton mentioned this pull request Sep 1, 2021

Tracking issue for Core Runtime HouseKeeping Items #58120

Closed

24 tasks

davidwrighton marked this pull request as ready for review September 1, 2021 22:01

davidwrighton requested review from BruceForstall and AndyAyersMS September 1, 2021 22:02

kasperk81 reviewed Sep 4, 2021

View reviewed changes

AndyAyersMS reviewed Sep 12, 2021

View reviewed changes

davidwrighton added 2 commits September 14, 2021 15:14

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

4078dc9

…rossgen_compiler

Address code review feedback

c90c3cd

- Undef all TARGET_ OS macros within the JIT so that they can't be used there - Convert the feature macros to inline functions. Should have equivalent perf on release/chk builds as macro approach, and is less problematic in general.

AndyAyersMS approved these changes Sep 15, 2021

View reviewed changes

BruceForstall requested changes Sep 16, 2021

View reviewed changes

First set of Bruce's feedback

efc7e20

Address SuperPMI and initialization feedback

a1b893f

BruceForstall reviewed Sep 17, 2021

View reviewed changes

davidwrighton added 4 commits September 21, 2021 17:32

Code review details from Bruce and Kunal

255c1f8

Update formatting

5bfd6ca

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

75d38c2

…rossgen_compiler

Add compUnixX86Abi function as needed in merge

94c30f5

BruceForstall approved these changes Sep 23, 2021

View reviewed changes

runfoapp bot mentioned this pull request Sep 24, 2021

emsdk failure: emcmake not found #59556

Closed

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

6c8cb2c

…rossgen_compiler

runfoapp bot mentioned this pull request Sep 27, 2021

System.Text.RegularExpressions.Tests.RegexMatchTests.Match_ExcessPrefix Failing in CI #59541

Closed

davidwrighton merged commit fcaba18 into dotnet:main Sep 28, 2021

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

davidwrighton deleted the universal_arm_crossgen_compiler branch April 13, 2023 18:49

	#define DEBUG_ARG_SLOTS_ASSERT(x) assert(compMacOsArm64Abi() \|\| x)
	#define DEBUG_ARG_SLOTS_ASSERT(x) assert(compMacOsArm64Abi() \|\| (x))

	assert(!TargetOS::IsWindows && !"Multireg store to SIMD reg not supported on Windows x64");
	assert(!TargetOS::IsWindows \|\| !"Multireg store to SIMD reg not supported on Windows x64");

		@@ -10,6 +10,14 @@

		#define fatMC // this is nice to have on so ildump works...

		CORINFO_OS g_currentOs = CORINFO_WINNT;

Universal arm crossgen compiler #58279

Universal arm crossgen compiler #58279

Conversation

davidwrighton commented Aug 27, 2021 • edited Loading

ghost commented Aug 27, 2021

davidwrighton commented Sep 1, 2021

davidwrighton commented Sep 1, 2021

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

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

davidwrighton commented Sep 15, 2021

AndyAyersMS left a comment

Choose a reason for hiding this comment

BruceForstall left a 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

davidwrighton commented Sep 16, 2021

BruceForstall commented Sep 16, 2021

davidwrighton commented Sep 16, 2021

kunalspathak commented Sep 17, 2021

BruceForstall left a 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

davidwrighton commented Aug 27, 2021 •

edited

Loading