Armel support in crossgen2 #43706

davidwrighton · 2020-10-21T21:21:30Z

This change adds support for armel target compilation in crossgen2

To use the feature compile an application using the --targetos Linux and --targetarch armel flags.

Summary of changes

To avoid bloating the compilation matrix, the #ifdefs used in the JIT that were conditional on ARM_SOFTFP behavior have been changed in to variable checks. To avoid perf penalties, this dynamic behavior is only enabled for the jit intended for compiling using crossgen2.
Armel abi processing was integrated into the various parts of the managed crossgen2 codebase that correspond to components with special handling in the coreclr codebase
- HFA support is disabled for Armel targets
- Floating point registers are not used when passing arguments

With this change it becomes possible to run crossgen2 on a Windows or Linux machine and target a Linux Armel target.

davidwrighton · 2020-10-21T21:25:46Z

@t-mustafin These are my changes to enable armel targetting from crossgen2. As I have no armel hardware or test environments, I have not tested the behavior of this.

@dotnet/jit-contrib This modifies the jit to be dynamically configurable for the ARM_SOFTFP and FEATURE_HFA ifdefs. I'd like some opinions on this approach, as I plan to make similar changes for the arm64 on OSX work that is ongoing, as well as possibly an attempt to compile a single arm and arm64 jit which targets both unix and Windows platforms.

BruceForstall

I'm intrigued by the ability to build a single binary to target multiple ABIs but still enable compile time specific code to avoid runtime overhead in some cases. I'm much more skeptical about something grander, such as arm/arm64 unification, compared to arm softfp, which is relatively small.

BruceForstall · 2020-10-22T22:23:54Z

src/coreclr/src/jit/codegencommon.cpp

+#ifdef UNIX_AMD64_ABI
+            if (true)
+#else  // !UNIX_AMD64_ABI
+            if (GlobalJitOptions::compFeatureHfa)
+#endif // UNIX_AMD64_ABI


Couldn't this just be:

#if !defined(UNIX_AMD64_ABI) if (GlobalJitOptions::compFeatureHfa) #endif // !UNIX_AMD64_ABI

Yes.

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

BruceForstall · 2020-10-22T22:29:42Z

src/coreclr/src/jit/compiler.cpp

+    opts.compUseSoftFP = !!JitConfig.JitSoftFP();
+    if (opts.compUseSoftFP)
+    {
+        GlobalJitOptions::compFeatureHfa = !opts.compUseSoftFP;


I'm not sure we want this as a static global. It seems like it needs to be a member of the Compiler object. What if we want to allow compiling both for arm and armel in the same process? Or if multiple threads try to initialize this concurrently?

Also, I don't think the config variable JitSoftFP should be the way this ABI is communicated to the JIT; it should come in via the jit flags, as a CorJitFlag.

The problem with compFeatureHfa is that it needs to be useable from the code in the code in LclVarDsc and fgArgTabEntry structures which don't have access to a Compiler object to query. Of course, I could pass the Compiler* as a parameter to these functions, but that didn't seem like a good approach. Given that the current model for usage of this is to only support one target architecture per process, I just went with a static. An alternative approach would be to use a threadstatic, which would allow for different compilations in the same process to have different values.

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

We already stash Compiler* in a thread static: JitTls::GetCompiler(), but I'm not sure it's used in non-DEBUG code.

BruceForstall · 2020-10-22T22:30:53Z

src/coreclr/src/jit/compiler.cpp

@@ -469,49 +474,6 @@ var_types Compiler::getJitGCType(BYTE gcType)
    return result;
 }

-#ifdef ARM_SOFTFP


I guess this appeared to be statically dead code?

Yes, this was only callable when FEATURE_HFA was true, except that ARM_SOFTFP also disables FEATURE_HFA.

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

BruceForstall · 2020-10-22T22:34:05Z

src/coreclr/src/jit/compiler.h

 #else
-        return false;
-#endif
+        NYI("GetLvHfaElemKind");


These NYI should be NOWAY, since they're never going to be implemented, it not that they are "not yet implemented".

BruceForstall · 2020-10-22T22:45:09Z

src/coreclr/src/jit/lower.cpp

@@ -3231,38 +3231,39 @@ void Lowering::LowerStoreLocCommon(GenTreeLclVarCommon* lclStore)
 //
 void Lowering::LowerRetStruct(GenTreeUnOp* ret)
 {
-#if defined(FEATURE_HFA) && defined(TARGET_ARM64)


Why remove the #if defined(TARGET_ARM64)? You've enabled this code for arm32. Is that correct? Maybe you can get away with it because we don't support SIMD on arm32.

I did not intend to make this change. Thank you for catching it.

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

BruceForstall · 2020-10-22T22:46:43Z

src/coreclr/src/jit/lower.cpp

+            // `call->HasMultiRegRetVal()` count double registers.
+            assert(comp->GetHfaCount(call) <= 2);
+#else  // !TARGET_ARM64 && !TARGET_ARM
+            NYI("Unknown architecture");


why change this from unreached() to NYI?

It turns out that part of the #if statement was never reached because of the #if which used to wrap it, and unreached() causes a compiler error now as the followon statements start producing errors. I'll change it to NOWAY from NYI per your other comment.

Upon reflection, this should actually probably remain as an NYI.

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

MichalStrehovsky · 2020-10-23T09:24:57Z

src/coreclr/src/tools/Common/TypeSystem/Common/TargetDetails.cs

@@ -42,6 +42,10 @@ public enum TargetAbi
        /// </summary>
        CoreRT,
        /// <summary>
+        /// model for armel execution model
+        /// </summary>
+        CoreRTArmel,


Did you consider making this a TargetArchitecture instead? A separate TargetArchitecture is how we had it until it got deleted because there was no use for it at that point.

We could put a helper on TargetDetails to help normalize those two into one for places that don't care about the distinction.

My thinking is that while hard/softfp distinction is only about the ABI, if we ever add support for soft - that's not just ABI anymore. I would just treat it as a very similar, but still a bit different CPU.

(I don't have strong opinion either way, it's just we still keep TargetAbi.Jit and if we ever resurrect that and have to add JitArmel for parity, it feels a bit confusing.)

This is all very confusing, and I agree its this isn't a very stable model within the compiler. The problem is that we tend to check for architecture with a simple equality check, and there are quite a few checks (and it would be really easy to get this wrong as we don't actively test this.).

With the abi, as this change is actually specific to the abi, and we don't have many abi checks in the product, this seemed safer and less risky going forward.

davidwrighton · 2020-10-23T18:58:34Z

What I meant by single arm and arm64 jits is that we would produce and arm targetting jit that handled all the various ABIs, Windows, Linux, Linux with Armel abi, and that we will generate a single arm64 jit, which targets Windows, Linux, and MacOS. I agree, merging the different architectures is a lost cause. Also, merging the X64 jit is a lot more work than the Arm one, as there are many more differences between Windows and Unix.

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

jashook · 2020-10-23T21:07:53Z

src/coreclr/src/jit/lclvars.cpp

@@ -2821,14 +2823,15 @@ void Compiler::makeExtraStructQueries(CORINFO_CLASS_HANDLE structHandle, int lev

 void Compiler::lvaSetStructUsedAsVarArg(unsigned varNum)
 {
-#ifdef FEATURE_HFA
+    if (GlobalJitOptions::compFeatureHfa)


I believe this if can be removed. The existing #ifdef for FEATURE_HFA was imo redundant.

BruceForstall

LGTM

t-mustafin · 2020-10-29T14:16:59Z

Sorry for a long answering.
I have run tests on armel, some tests start to fail, some tests runs too long time.

Results on master_0e402bc + cherry-picked 1st_commit and 2nd_commit:
patched.tar.gz

Results on master_0e402bc for reference:
orig.tar.gz

Unfortunatelly patched.tar.gz is not a full log, cause I do not got it yet. I will restart testing with all patches from the PR and more recent master.

src/coreclr/src/zap/zapinfo.cpp

davidwrighton · 2020-10-29T22:00:10Z

Ah, ok, from the logs it appears that the way you are using crossgen2, you are not using the --targetarch switch to pass armel to crossgen2. And, unlike all of the other architectures, when run locally, armel isn't autodetected (as there is no platform api that will provide that bit of information.). Fortunately, I believe that is pretty simple to fix. I'll fix that, as well as the build break you noticed.

davidwrighton · 2020-10-29T22:37:21Z

@t-mustafin Actually, I've just fixed the build break. I don't believe I fully understand the means by which you are running crossgen2, and the environment. As of the last PR you submitted, I believe that crossgen2 should be failing while trying to load libjitinterface_arm.so when you actually are building libjitinterface_armel.so. Did you do something like add a new enum value to the Architecture enum in System.InteropServices.RuntimeInformation that is Armel? In any case, you should be passing the --targetarch switch to crossgen2 when compiling.

t-mustafin · 2020-11-02T10:53:23Z

Two tests starts to fail:

 JIT/Methodical/Invoke/ctor/_dbgval_ctor/_dbgval_ctor.sh                                      
 JIT/Regression/JitBlue/DevDiv_462274/DevDiv_462274/DevDiv_462274.sh

One test starts to pass, looking at another runs seems it is unstable:

JIT/jit64/regress/ddb/113574/113574/113574.sh

Results on master_7d15ecf + cherry picked commits 1 2 3 4 5 6
patched.tar.gz
Results on master_7d15ecf:
results.tar.gz

Also I would like to run tests after cross compiling on x64 to armel, but not done it yet.

t-mustafin · 2020-11-05T12:58:57Z

Results with cross generated ni files on x64.

Libs failed to cross-generate, that generates on native-run:

Microsoft.CodeAnalysis.CSharp.ni.dll
Microsoft.CodeAnalysis.VisualBasic.ni.dll
R2RTest/Microsoft.Build.ni.dll

Tests was failed to cross-generate, that native-generated successfully:

./JIT/Regression/JitBlue/GitHub_25027/GitHub_25027/GitHub_25027.ni.dll

Tests was failed after cross-generate, that pass after native-generate:

ilasm/PortablePdb/IlasmPortablePdbTests/IlasmPortablePdbTests.sh
ilasm/System/Runtime/CompilerServices/MethodImplOptionsTests/MethodImplOptionsTests.sh
readytorun/tests/mainv1/mainv1.sh
readytorun/tests/mainv2/mainv2.sh

Roughly it looks good: 11 tests fails + 13 crossgen fails compared with 112 fails on master branch.
cross.tar.gz

Does native-generated and cross-generated ni files should be equal? I got different hashes for it.
Native:

# ./corerun ./crossgen2/crossgen2.dll -r:./*.dll -r:./crossgen2/*.dll --targetarch armel  -o:/opt/usr/root/hello.ni.dll  /opt/usr/root/hello.dll
Emitting R2R PE file: /opt/usr/root/hello.ni.dll
# sha1sum /opt/usr/root/hello.dll /opt/usr/root/hello.ni.dll
918acdf2054cb2e96668a01bb57610a789c0c25a  /opt/usr/root/hello.dll
9103f3dbe325b3f208d5f2b77899c5e794cad5e6  /opt/usr/root/hello.ni.dll

Cross:

$ ./corerun ./crossgen2/crossgen2.dll -r:./*.dll -r:./crossgen2/*.dll --targetarch armel  -o:/home/tmustafin/hello/hello.ni.dll  /home/tmustafin/hello/hello.dll
Emitting R2R PE file: /home/tmustafin/hello/hello.ni.dll
$ sha1sum /home/tmustafin/hello/hello.dll /home/tmustafin/hello/hello.ni.dll
918acdf2054cb2e96668a01bb57610a789c0c25a  /home/tmustafin/hello/hello.dll
9ae5debe46ac00e036fe6d68ac8d03a725a61906  /home/tmustafin/hello/hello.ni.dll

hello.tar.gz

cc @alpencolt

AntonLapounov · 2020-11-24T06:07:53Z

src/coreclr/src/jit/jit.h

@@ -514,7 +535,7 @@ typedef ptrdiff_t ssize_t;

 #if DUMP_GC_TABLES
 #pragma message("NOTE: this non-debug build has GC ptr table dumping always enabled!")
-const bool dspGCtbls = true;
+const bool            dspGCtbls      = true;


I think you did not mean to insert those spaces.

ViktorHofer · 2020-12-08T14:51:30Z

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

mangod9 · 2021-02-23T05:44:07Z

@davidwrighton @t-mustafin assume we should still merge these changes? Is there anything blocking other than resolving conflicts since its a been a few months since this was opened?

t-mustafin · 2021-02-25T13:56:44Z

@davidwrighton @t-mustafin assume we should still merge these changes? Is there anything blocking other than resolving conflicts since its a been a few months since this was opened?

Yes, we need this changes. I have already appreved it, let it merge please.

mangod9 · 2021-03-04T02:21:32Z

since this has been dormant for a few months, the conflicts need some manual resolving. @davidwrighton if you think they should be straightforward I can have someone shepherd this through.

clamp03 · 2021-03-09T02:55:50Z

@davidwrighton Could you look into #49347 ?

…armhf and armel calling conventions Crossgen2 managed code changes to support Armel abi - Adjust handling for armel target architecture command line handling to capture that armel is the abi - Tweak field layout to disable computation of hfa status for valuetypes - Inform JIT that armel abi is to be used - Add armel abi value to TargetAbi enum Checkin requested changes Fix build on Unix platforms Make the JIT built for crossgen2 be dynamically configurable between armhf and armel calling conventions Crossgen2 managed code changes to support Armel abi - Adjust handling for armel target architecture command line handling to capture that armel is the abi - Tweak field layout to disable computation of hfa status for valuetypes - Inform JIT that armel abi is to be used - Add armel abi value to TargetAbi enum Checkin requested changes Fix build on Unix platforms Reformat the code Fix build break Fix Windows build Code review - Use static variable, but protect from misuse with NO_WAY - Pass info to jit via jitflag, not via jit config tweak variable type Address jit format issue Fix build break

Dotnet-GitSync-Bot added the area-crossgen2-coreclr label Oct 21, 2020

davidwrighton requested a review from BruceForstall October 22, 2020 19:13

BruceForstall requested changes Oct 22, 2020

View reviewed changes

MichalStrehovsky reviewed Oct 23, 2020

View reviewed changes

jashook reviewed Oct 23, 2020

View reviewed changes

davidwrighton force-pushed the armel_support_crossgen2 branch from b2c7f1e to f911918 Compare October 26, 2020 23:42

This was referenced Oct 27, 2020

OSX deprovision jaredpar/runfo#41

Closed

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

BruceForstall approved these changes Oct 28, 2020

View reviewed changes

t-mustafin reviewed Oct 29, 2020

View reviewed changes

src/coreclr/src/zap/zapinfo.cpp Outdated Show resolved Hide resolved

AntonLapounov reviewed Nov 24, 2020

View reviewed changes

t-mustafin approved these changes Jan 14, 2021

View reviewed changes

Base automatically changed from master to main March 1, 2021 09:07

clamp03 mentioned this pull request Mar 9, 2021

armel build fail #49347

Closed

davidwrighton added 2 commits March 15, 2021 11:25

Fix build break

e92b859

davidwrighton force-pushed the armel_support_crossgen2 branch from 4221196 to e92b859 Compare March 15, 2021 18:32

Use correct transition block

011ab02

This was referenced Mar 16, 2021

Test failure Interop/ICustomMarshaler/Primitives/ICustomMarshaler_TargetUnix/ICustomMarshaler_TargetUnix.sh #49621

Closed

interop/icustommarshaler/primitives/icustommarshaler_targetunix/icustommarshaler_targetunix.sh #49724

Closed

davidwrighton merged commit c28f3ad into dotnet:main Mar 17, 2021

ghost locked as resolved and limited conversation to collaborators Apr 16, 2021

davidwrighton deleted the armel_support_crossgen2 branch April 20, 2021 17:47

karelz added this to the 6.0.0 milestone May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Armel support in crossgen2 #43706

Armel support in crossgen2 #43706

davidwrighton commented Oct 21, 2020

davidwrighton commented Oct 21, 2020

BruceForstall left a comment

BruceForstall Oct 22, 2020

davidwrighton Oct 23, 2020

BruceForstall Oct 22, 2020

davidwrighton Oct 23, 2020

BruceForstall Oct 23, 2020

BruceForstall Oct 22, 2020

davidwrighton Oct 23, 2020

BruceForstall Oct 22, 2020

BruceForstall Oct 22, 2020

davidwrighton Oct 23, 2020

BruceForstall Oct 22, 2020

davidwrighton Oct 23, 2020

davidwrighton Oct 23, 2020

MichalStrehovsky Oct 23, 2020

MichalStrehovsky Oct 23, 2020

davidwrighton Oct 23, 2020

davidwrighton commented Oct 23, 2020

jashook Oct 23, 2020

BruceForstall left a comment

t-mustafin commented Oct 29, 2020

davidwrighton commented Oct 29, 2020

davidwrighton commented Oct 29, 2020

t-mustafin commented Nov 2, 2020

t-mustafin commented Nov 5, 2020

AntonLapounov Nov 24, 2020

ViktorHofer commented Dec 8, 2020

mangod9 commented Feb 23, 2021

t-mustafin commented Feb 25, 2021

mangod9 commented Mar 4, 2021

clamp03 commented Mar 9, 2021

Armel support in crossgen2 #43706

Armel support in crossgen2 #43706

Conversation

davidwrighton commented Oct 21, 2020

davidwrighton commented Oct 21, 2020

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

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 Oct 23, 2020

Choose a reason for hiding this comment

BruceForstall left a comment

Choose a reason for hiding this comment

t-mustafin commented Oct 29, 2020

davidwrighton commented Oct 29, 2020

davidwrighton commented Oct 29, 2020

t-mustafin commented Nov 2, 2020

t-mustafin commented Nov 5, 2020

Choose a reason for hiding this comment

ViktorHofer commented Dec 8, 2020

mangod9 commented Feb 23, 2021

t-mustafin commented Feb 25, 2021

mangod9 commented Mar 4, 2021

clamp03 commented Mar 9, 2021