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

Add ability for crossgen2 to synthesize PGO histograms #77683

Merged
merged 38 commits into from
Nov 17, 2022

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Oct 31, 2022

Add an option --synthesize-random-mibc. When passed, crossgen2 will use
metadata from the compilation group to synthesize PGO histograms for
methods that did not have any input profile data. Also supported for --embed-pgo-data,
in which case the synthesized PGO data gets embedded.

Adds a synthesizepgo argument to the src/tests/run.cmd/src/tests/run.sh scripts which enable this mode for compatible tests. So testing can be done locally via:

.\src\tests\build.cmd crossgen2 checked
.\src\tests\run.cmd checked runcrossgen2tests synthesizepgo

This passes in pri1 on win-x64 for me currently. I have not yet hooked it up in CI. I want to do that in a separate change later, for now this is enough to allow me to do some manual testing on #77267 that has at least some level of coverage.

Some stats:

  • crossgen'd SPC, static PGO: 1170 type-based GDVs
  • crossgen'd SPC, synthesis: 8400 type-based GDVs
  • crossgen'd libraries, static PGO: 2362 type-based GDVs
  • crossgen'd libraries, synthesis: 33061 type-based GDVs
  • Priority 1 tests with synthesized PGO: 19450 type-based GDVs

Ideally we can get the libraries tests running with crossgen'd libraries using synthesized PGO, but it will depend on #75230, so I will leave it to future work.

Add an option --synthesize-random-mibc. When passed, crossgen2 will use
metadata from the compilation group to synthesize PGO histograms for
methods that did not have any input profile data.

Mainly for testing purposes. Currently only works in R2R mode, but
hopefully will also work to embed random static PGO data in the future.
@jakobbotsch jakobbotsch force-pushed the crossgen2-synthesized-pgo branch from 518d473 to 255a128 Compare October 31, 2022 16:17
Split up "we have a profile" and "we have profile weights" questions.
Avoid ordering issues. Also revert now unrelated PhasedVar changes.
@runfoapp runfoapp bot mentioned this pull request Nov 2, 2022
Various places were setting this to "true" and various other places to
"1". Always use "1" in the environment variable and "true" in the
msbuild property. This fixes local runs of crossgen2 tests (using
src/tests/run.cmd runcrossgen2tests) -- these were not skipping
FileCheck tests before as the test wrapper script only checks for "1".
Comment on lines -64 to -66

// R2R field layout needs compilation group information
((ReadyToRunCompilerContext)context).SetCompilationGroup(group);
Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved this so it happens right after we create the compilation group so it is early enough for the ProfileDataManager that also needs this now (for some EnsureLoadable calls).

@jakobbotsch jakobbotsch marked this pull request as ready for review November 7, 2022 22:07
Comment on lines +4 to +5
<!-- Synthesized PGO may cause the expected TypeLoadExceptions in this test to appear at unexpected places -->
<SynthesizedPgoIncompatible>true</SynthesizedPgoIncompatible>
Copy link
Member Author

@jakobbotsch jakobbotsch Nov 7, 2022

Choose a reason for hiding this comment

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

This is probably an actual bug -- loading the type for the GDV guess can have observable behavior due to load failure, and does so in this test. Ideally we would have a "TypeHandle or null if not loadable" fixup we could use for GDV guesses, but for now I have just disabled this single test.

Copy link
Member

Choose a reason for hiding this comment

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

Please file this issue as a bug and push for it to get fixed.

@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @davidwrighton

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

:shipit:

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jakobbotsch jakobbotsch merged commit 6babe56 into dotnet:main Nov 17, 2022
@jakobbotsch jakobbotsch deleted the crossgen2-synthesized-pgo branch November 17, 2022 21:27
@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants