Remove ALT_JIT define and replace with runtime logic #44565

davidwrighton · 2020-11-12T00:38:32Z

The ALT_JIT define was causing confusion and problems when used in crossgen2

It interfered with capturing jit dumps
Didn't translate nicely to CoreRT codebase

This change replaces all of the existing #ifdef logic with a dynamic check of a jit flag.

davidwrighton · 2020-11-12T00:39:35Z

@dotnet/jit-contrib
@BruceForstall, I know we discussed getting rid of the ALT_JIT ifdef. Is this the approach you were thinking of? Also, as I don't actually know what all the scenarios are for altjit, I need someone on the JIT team to test them for me.

echesakov · 2020-11-12T00:51:29Z

@dotnet/jit-contrib Assuming that I understand the main value of altjits - reproducing the assertions on cross platforms (x64 -> arm64, x86 -> arm, win-x64 -> linux-x64) I wonder if it is worth defining the altjit related checks under DEBUG ifdefs in order to minimize impact on the release jit.

@davidwrighton Also, as I don't actually know what all the scenarios are for altjit, I need someone on the JIT team to test them for me.

We stopped running altjit in the CI a while ago. You can run the tests locally to verify the changes.
You would need to specify:

COMPlus_AltJitName=<altjit.dll>
COMPlus_AltJit=*

BruceForstall · 2020-11-12T01:25:03Z

This should fix #41643

We've used altjits in the past for:

reproducing asserts or debugging the JIT for cross-platform scenarios where it's easier to debug on some platforms (Windows x64) than others (arm64/32). This is the most common case.
bringing up a new compiler, either same arch or cross arch. E.g., bringing up RyuJIT was done this way as an altjit over JIT32/64.
for SPMI to avoid moving this existing JIT dll. IMO, this is a hack that has caused lots of problems and confusion.
to ship the RyuJIT CTPs.
for debugging same-platform JIT and using the COMPlus_AltJit options to enable your change for a subset of code. Could be useful for binary searching between your changes and a baseline.

CarolEidt · 2020-11-12T01:26:51Z

My main use of altjits is in running diffs, since it's much easier to use a single platform to get diffs for multiple targets.

davidwrighton · 2020-11-12T01:32:04Z

I think my changes address everything but the SPMI scenario. Bruce, could you elaborate on that one and give me some guidance on how one might handle that case?

BruceForstall · 2020-11-12T01:51:30Z

For SPMI, we need to introduce the SPMI shim between the JIT and VM. Traditionally, we would simply move the clrjit.dll someplace else, then copy the shim to the VM location as clrjit.dll, setting some variables so the shim can find the original clrjit.dll. It's annoying to move / rename files, so now we set the altjit variables to the SPMI shim, and then the shim loads the clrjit.dll (or potentially some cross-plat compiler) in its usual place. The annoying thing is we capture the COMPlus variables into the replay log, and we need to force them off when doing a SPMI replay.

AndyAyersMS · 2020-11-12T02:05:03Z

jit-diff-pmi does a similar swapping of jits (moving checked base/diff jits in place of the base release jit), and allowing a normal jit to function as an altjit would simplify the workflow there too.

davidwrighton · 2020-11-12T02:16:42Z

@AndyAyersMS This change makes all jits capable of acting like an altjit, on a per-method basis. From what you say, this should just work then.

@BruceForstall So, for SPMI, the fix would be to ignore the altjit jit flag when it comes from the VM, and add some other sort of config flag for SPMI to identify if it should treat the jit it wraps as being an alt jit? Is that what we would need here?

BruceForstall

Generally looks great. Thanks for doing this!

src/coreclr/src/inc/corjitflags.h

src/coreclr/src/vm/jitinterface.cpp

BruceForstall · 2020-11-12T02:05:09Z

src/coreclr/src/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilationBuilder.cs

@@ -74,9 +74,6 @@ public override CompilationBuilder UseBackendOptions(IEnumerable<string> options
                builder.Add(new KeyValuePair<string, string>(name, value));
            }

-            // As we always use an AltJit to compile, tell the jit to always generate code
-            builder.Add(new KeyValuePair<string, string>("AltJitNgen", "*"));


src/coreclr/src/jit/error.h

BruceForstall · 2020-11-12T02:29:39Z

src/coreclr/src/jit/error.h

@@ -153,24 +154,17 @@ extern void RecordNowayAssertGlobal(const char* filename, unsigned line, const c
 #define unreached() noWayAssertBody()

 #define NOWAY_MSG(msg) noWayAssertBodyConditional(NOWAY_ASSERT_BODY_ARGUMENTS)
+#define NOWAY_MSG_FILE_AND_LINE(msg, file, line) noWayAssertBodyConditional(NOWAY_ASSERT_BODY_ARGUMENTS)


I guess file and line are ignored here, in favor of __FILE__, __LINE__

Yes. Well, sortof. This is the release portion of the ifdef, where FILE and LINE aren't used at all. The goal is to match the overall behavior of what existed before.

In particular, non-altjits don't put in file/line number stuff, but chk/dbg will.

BruceForstall · 2020-11-12T02:46:04Z

So, for SPMI, the fix would be to ignore the altjit jit flag when it comes from the VM, and add some other sort of config flag for SPMI to identify if it should treat the jit it wraps as being an alt jit? Is that what we would need here?

That would work. I'm not sure we need to worry about the scenario where SPMI collects using an altjit, but if so, maybe we need SPMI-specific flags to set altjit in that case.

I think we need a way to tell the VM to load a different primary JIT -- I don't want the SPMI shim to be loaded as an "altjit". We actually have COMPlus_JitName which we should use in ExecutionManager::GetJitName(). I fact, I think that's exactly what it used to do in desktop, but it appears in core someone removed the usage even though COMPlus_JitName still lives in clrconfigvalues.h.

btw, none of these proposed SPMI changes need to happen in this PR.

BruceForstall · 2020-11-12T20:19:51Z

I built and tested basic JIT and NGEN altjit asm dumps (with the flags fix I showed) and it works fine.

Can you add the following to CodeGen::genGenerateMachineCode:

        if (compiler->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_ALT_JIT))
        {
            printf("; invoked as altjit\n");
        }

at the end of the if (compiler->opts.disAsm) case? Then we can see in the asm output that this was an altjit compile.

Note also that you don't need the opts.compAltJitRequested member variable at all: you can always instead use the expression opts.jitFlags->IsSet(JitFlags::JIT_FLAG_ALT_JIT).

BruceForstall · 2020-11-13T00:04:32Z

It looks to me like we need a change to SuperPMI to match this change.

Currently, when we do SPMI collection, we do so by setting AltJit to the SPMI shim. This causes the collection to capture the COMPlus_AltJit variables. On replay, we generally pass the (annoying) options -jitoption force AltJit= -jitoption force AltJitNgen= to force the environment to set these variables to empty (overriding the collection), despite the fact they are defined in the collected environment variables. However, that doesn't work anymore, because the altjit flag is captured and replayed in the JIT flags, and our -jitoption to override the environment does not override the jit flags. So, we will always tell the replay JIT that it's an altjit. However, since COMPlus_AltJit is forced empty, the "altjit" will always say it isn't responsible for compiling the replay.

This implies something about how this is implemented. Currently, the JIT looks to see if COMPlus_AltJit is set, and if so, if the function we are compiling now matches what is specified, and sets opts.altJit = true if so. If COMPlus_AltJit=*, it matches any function (this is the only option for release builds). If COMPlus_AltJit=MyMain, it only invokes the altjit on MyMain, otherwise fails with CORJIT_SKIPPED. But presumably, all this logic should happen in the VM since if we get invoked with the new altjit jit flag, we should assume we're going to compile it in altjit "mode". And SPMI should probably just clear the altjit flag bit before invoking the JIT.

BruceForstall · 2020-11-13T01:12:11Z

@davidwrighton I submitted a PR that fixes the SPMI issue: davidwrighton#1

@sandreenko Take a look; my PR is kind of a hack. I think we need to rethink how SPMI collections get invoked w.r.t. the altjit flags.

sandreenko · 2020-11-13T20:31:53Z

@sandreenko Take a look; my PR is kind of a hack. I think we need to rethink how SPMI collections get invoked w.r.t. the altjit flags.

Do we set altjit flag during collection for CoreRun nowadays? For crossgen2 we pass a path to the invoked jit, so there we don't use altjit flags.

BruceForstall · 2020-11-13T23:26:18Z

Do we set altjit flag during collection for CoreRun nowadays?

Yes, in both superpmi.py and the src\tests\JIT\superpmi>src\tests\JIT\superpmi\superpmicollect.cs, we set it for collection, and force clear it on replay.

Fix SuperPMI for new altjit jit flag

davidwrighton · 2020-11-16T21:28:14Z

@BruceForstall I've merged your change, and responded to feedback. Please re-review.

BruceForstall

LGTM

BruceForstall · 2020-11-17T03:32:25Z

Don't you need to rebase instead of merge (from master)?

jkotas

Nice!

Remove ALT_JIT define and replace with runtime logic

661b151

davidwrighton requested a review from BruceForstall November 12, 2020 00:38

Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 12, 2020

BruceForstall requested changes Nov 12, 2020

View reviewed changes

BruceForstall reviewed Nov 12, 2020

View reviewed changes

Fix code review and formatting issues

b191fba

runfoapp bot mentioned this pull request Nov 12, 2020

Errors installing the SDK during builds #34015

Closed

Fix SuperPMI for new altjit jit flag

962cc86

davidwrighton added 2 commits November 16, 2020 13:20

Merge pull request #1 from BruceForstall/FixSPMI

6c4ede5

Fix SuperPMI for new altjit jit flag

Code review feedback

7faeb19

BruceForstall approved these changes Nov 16, 2020

View reviewed changes

Merge branch 'master' into remove_alt_jit_define

3cce8e2

jkotas approved these changes Nov 17, 2020

View reviewed changes

jkotas merged commit 860eaf1 into dotnet:master Nov 17, 2020

BruceForstall mentioned this pull request Nov 23, 2020

Stop building JIT as altjit or not altjit #41643

Closed

ghost locked as resolved and limited conversation to collaborators Dec 17, 2020

davidwrighton deleted the remove_alt_jit_define branch April 20, 2021 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ALT_JIT define and replace with runtime logic #44565

Remove ALT_JIT define and replace with runtime logic #44565

davidwrighton commented Nov 12, 2020

davidwrighton commented Nov 12, 2020

echesakov commented Nov 12, 2020 •

edited

Loading

BruceForstall commented Nov 12, 2020

CarolEidt commented Nov 12, 2020

davidwrighton commented Nov 12, 2020

BruceForstall commented Nov 12, 2020

AndyAyersMS commented Nov 12, 2020

davidwrighton commented Nov 12, 2020

BruceForstall left a comment

BruceForstall Nov 12, 2020

BruceForstall Nov 12, 2020

davidwrighton Nov 12, 2020

davidwrighton Nov 12, 2020

BruceForstall commented Nov 12, 2020 •

edited

Loading

BruceForstall commented Nov 12, 2020

BruceForstall commented Nov 13, 2020

BruceForstall commented Nov 13, 2020

sandreenko commented Nov 13, 2020

BruceForstall commented Nov 13, 2020

davidwrighton commented Nov 16, 2020

BruceForstall left a comment

BruceForstall commented Nov 17, 2020

jkotas left a comment

Remove ALT_JIT define and replace with runtime logic #44565

Remove ALT_JIT define and replace with runtime logic #44565

Conversation

davidwrighton commented Nov 12, 2020

davidwrighton commented Nov 12, 2020

echesakov commented Nov 12, 2020 • edited Loading

BruceForstall commented Nov 12, 2020

CarolEidt commented Nov 12, 2020

davidwrighton commented Nov 12, 2020

BruceForstall commented Nov 12, 2020

AndyAyersMS commented Nov 12, 2020

davidwrighton commented Nov 12, 2020

BruceForstall left a comment

Choose a reason for hiding this comment

BruceForstall Nov 12, 2020

Choose a reason for hiding this comment

BruceForstall Nov 12, 2020

Choose a reason for hiding this comment

davidwrighton Nov 12, 2020

Choose a reason for hiding this comment

davidwrighton Nov 12, 2020

Choose a reason for hiding this comment

BruceForstall commented Nov 12, 2020 • edited Loading

BruceForstall commented Nov 12, 2020

BruceForstall commented Nov 13, 2020

BruceForstall commented Nov 13, 2020

sandreenko commented Nov 13, 2020

BruceForstall commented Nov 13, 2020

davidwrighton commented Nov 16, 2020

BruceForstall left a comment

Choose a reason for hiding this comment

BruceForstall commented Nov 17, 2020

jkotas left a comment

Choose a reason for hiding this comment

echesakov commented Nov 12, 2020 •

edited

Loading

BruceForstall commented Nov 12, 2020 •

edited

Loading