Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SPMI: Ensure we disable loop alignment consistently #82100

Conversation

jakobbotsch
Copy link
Member

Recently we disabled loop alignment in SPMI diffs, but this was only done for the SPMI invocations when we generate disassembly. This would result in the diffs run seeing different generated code than subsequent disassembly runs.

This splits the environment variables we want to have enabled in the diffs SPMI invocation out into its own dictionary. These are force overridden.

Recently we disabled loop alignment in SPMI diffs, but this was only
done for the SPMI invocations when we generate disassembly. This would
result in the diffs run seeing different generated code than subsequent
disassembly runs.

This splits the environment variables we want to have enabled in the
diffs SPMI invocation out into its own dictionary. These are force
overridden.
@ghost ghost assigned jakobbotsch Feb 14, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 14, 2023
@ghost
Copy link

ghost commented Feb 14, 2023

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

Issue Details

Recently we disabled loop alignment in SPMI diffs, but this was only done for the SPMI invocations when we generate disassembly. This would result in the diffs run seeing different generated code than subsequent disassembly runs.

This splits the environment variables we want to have enabled in the diffs SPMI invocation out into its own dictionary. These are force overridden.

Author: jakobbotsch
Assignees: jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @kunalspathak @BruceForstall

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM. How was this caught? Also, what is "force" about? I see its reference in other places in the file too.

@jakobbotsch
Copy link
Member Author

LGTM. How was this caught? Also, what is "force" about? I see its reference in other places in the file too.

I noticed when I did an asmdiffs run that the diffs reported by jit-analyze did not sum to the one reported by superpmi.py.

Force is used to override JIT env vars that were recorded into the context. For example, if we in the future record a test with DOTNET_JitAlignLoops=1, then the "force" allows this to be overridden to DOTNET_JitAlignLoops=0 in asmdiffs runs.

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr superpmi-diffs

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@jakobbotsch
Copy link
Member Author

@jakobbotsch
Copy link
Member Author

Hmm, there are two same size diffs in the SPMI-diffs run, but the .dasm files do not have any diffs. Will investigate.

@jakobbotsch
Copy link
Member Author

jakobbotsch commented Feb 15, 2023

That happens on main too. It seems there is some issue with recording relocations, the diffs that SPMI is finding is in constants that should have been recorded as relocations. When I look at the dump, the offsets for those relocations are off by 0x10 bytes.
I wonder if it also explains some of the recent regressions, e.g. #82103 (comment), that looks like they could be loop alignment related.

@jakobbotsch
Copy link
Member Author

I will merge this and continue investigating the relocation issue separately.

@jakobbotsch jakobbotsch merged commit 3a36920 into dotnet:main Feb 15, 2023
@jakobbotsch jakobbotsch deleted the superpmi-disable-loop-alignment-consistently branch February 15, 2023 10:38
@jakobbotsch
Copy link
Member Author

Hmm, actually no, there is no relocation emitted for that particular constant I was looking at, which is probably the problem:

compProfilerMethHnd = (void*)DummyProfilerELTStub;

A newly added test has DOTNET_JitStress=1 and it causes us to insert these enter/leave calls to this method, which resides in clrjit.dll and will be different for base/diff JIT.

@BruceForstall
Copy link
Member

A newly added test has DOTNET_JitStress=1 and it causes us to insert these enter/leave calls to this method, which resides in clrjit.dll and will be different for base/diff JIT.

Maybe we should fix the JIT to emit relocations.

However, we generally shouldn't have tests that set DOTNET_JitStress, since the mix of stress modes invoked might change based on the test hash (theoretically this shouldn't change unless the test is altered).

Looks like the tests are:

JIT\Regression\JitBlue\Runtime_81585\Runtime_81585.csproj
JIT\Regression\JitBlue\Runtime_81725\Runtime_81725.csproj

Instead, these should use DOTNET_JitStressModeNames and set exactly the stress modes needed for the test.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants