Skip to content

Introduce DOTNET_AltJitOS - #64979

Merged
EgorBo merged 6 commits into
dotnet:mainfrom
EgorBo:add-altjitos
Feb 9, 2022
Merged

Introduce DOTNET_AltJitOS#64979
EgorBo merged 6 commits into
dotnet:mainfrom
EgorBo:add-altjitos

Conversation

@EgorBo

@EgorBoEgorBo commented Feb 8, 2022•
edited
Loading

Copy link
Copy Markdown
Member

#58279 merged all arm64 jits into a single universal binary. This PR adds back ability to specify target OS for ARM/ARM64 in order to test ABI specifics, maybe in future we'll merge desktop alt jits too (or all of them).
Example:

DOTNET_AltJitName=clrjit_universal_arm64_x64.dll
DOTNET_AltJitOS=macOS

to validate ios-arm64 ABI on windows host

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 8, 2022
@ghostghost assigned EgorBoFeb 8, 2022
@ghost

ghost commented Feb 8, 2022

Copy link
Copy Markdown

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

Issue Details

#58279 merged all arm64 jits into a single universal binary. This PR adds back ability to specify target OS for ARM/ARM64, maybe in future we'll merge desktop alt jits too (or all of them).

Author:EgorBo
Assignees:EgorBo
Labels:

area-CodeGen-coreclr

Milestone:-

jakobbotsch
jakobbotsch reviewed Feb 8, 2022
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
@jakobbotschjakobbotsch added area-VM-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Feb 8, 2022
am11
am11 reviewed Feb 8, 2022
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
am11
am11 reviewed Feb 8, 2022
View reviewed changes
Comment threadsrc/coreclr/vm/codeman.cpp Outdated
@am11

am11 commented Feb 8, 2022

Copy link
Copy Markdown
Member

DOTNET_AltJitOs

nit: typical convention is to use uppercase for two letters: DOTNET_AltJitOS (but we don't follow it everywhere). :)

@EgorBo

EgorBo commented Feb 8, 2022

Copy link
Copy Markdown
MemberAuthor

DOTNET_AltJitOs

nit: typical convention is to use uppercase for two letters: DOTNET_AltJitOS (but we don't follow it everywhere). :)

it feels weird, because JIT is also an abbreviation, so it should be either JitOs or JITOS. Anyway, it's case insensitive :)

@EgorBo@am11
Update src/coreclr/vm/codeman.cpp
7d78b7b
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@am11

am11 commented Feb 8, 2022

Copy link
Copy Markdown
Member

Yes, for more than two letters, it follows PascalCase (TargetOS, Xml and so on).

@EgorBo

EgorBo commented Feb 8, 2022

Copy link
Copy Markdown
MemberAuthor

Yes, for more than two letters, it follows PascalCase (TargetOS, Xml and so on).

Ok, changed to OS 🙂

@EgorBoEgorBo changed the title Introduce DOTNET_AltJitOsIntroduce DOTNET_AltJitOSFeb 8, 2022
@runfoapprunfoappBot mentioned this pull request Feb 8, 2022
Closed
@EgorBo

EgorBo commented Feb 9, 2022

Copy link
Copy Markdown
MemberAuthor

@dotnet/jit-contrib @jakobbotsch PTAL

@EgorBo
EgorBo merged commit baae8fa into dotnet:mainFeb 9, 2022
BruceForstall
BruceForstall reviewed Feb 9, 2022
View reviewed changes

@BruceForstallBruceForstall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add support for something like this in SuperPMI?

Currently, SPMI calls setTargetOS() with the OS that was used in collection. Do we need the ability to override that? E.g., collect on linux-arm64, replay targeting macos-arm64? It might not work anyway, if that causes different JIT/EE interface calls.

Comment threadsrc/coreclr/inc/clrconfigvalues.h
#if defined(ALLOW_SXS_JIT)
RETAIL_CONFIG_STRING_INFO(EXTERNAL_AltJitName, W("AltJitName"), "Alternative Jit to use, will fall back to primary jit.")
RETAIL_CONFIG_STRING_INFO(EXTERNAL_AltJit, W("AltJit"), "Enables AltJit and selectively limits it to the specified methods.")
RETAIL_CONFIG_STRING_INFO(EXTERNAL_AltJitOs, W("AltJitOS"), "Sets target OS for AltJit or uses native one by default. Only applicable for ARM/AMR64 at the moment.")

@BruceForstallBruceForstallFeb 9, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would use EXTERNAL_AltJitOS so the name exactly matches (including case) the variable name.

Also: typo: AMR64 => ARM64

@EgorBoEgorBoFeb 14, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, will fix in a follow up!

Regarding SuperPMI - not sure, I just wanted to test ABI differences on ARM64

@ghostghost locked as resolved and limited conversation to collaborators Mar 16, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jakobbotschjakobbotschjakobbotsch approved these changes
+2 more reviewers
@am11am11am11 left review comments
@BruceForstallBruceForstallBruceForstall left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@EgorBoEgorBo

Labels

area-VM-coreclr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@EgorBo@am11@jakobbotsch@BruceForstall