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

Fix Issue 22861 - build the compiler with PGO #13791

Closed
wants to merge 1 commit into from

Conversation

maxhaton
Copy link
Member

@maxhaton maxhaton commented Mar 9, 2022

This introduces a new target to build.d, dmd-pgo, which will build a dmd with PGO
instrumentation, then run either the phobos or dmd testsuite (both are present in the
source code, only the dmd one is set to run at the moment). This data is then merged,
at which point the data is used to build a release + LTO build of dmd.

This resulting build is significantly faster - on the order of 45% on some programs.

ENABLE_PGO=1 can be used to make the build script use the dmd-pgo by default, however
something more subtle may be required to make all dmd builds (i.e. releases)
fully pgo-ified

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @maxhaton!

Bugzilla references

Auto-close Bugzilla Severity Description
22861 enhancement Build the compiler with PGO

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#13791"

src/build.d Outdated Show resolved Hide resolved
src/build.d Outdated Show resolved Hide resolved
src/build.d Show resolved Hide resolved
const scope cmd = [ testRunner.targets[0], "compilable", "-j" ~ jobs.to!string ];
log("%-(%s %)", cmd);
if (spawnProcess(cmd, null, Config.init, testDir).wait())
stderr.writeln("dmd tests failed! This will not end the PGO build because some data may have been gathered");
Copy link
Contributor

Choose a reason for hiding this comment

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

Test failures are allowed because of missing tools?

Copy link
Member Author

Choose a reason for hiding this comment

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

Which missing tools? If something has gone catastrophically wrong then I guess the build should stop but frankly I view that as unlikely versus most tests passing but some failing due to CI flakyness

Copy link
Contributor

Choose a reason for hiding this comment

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

Which missing tools?

Whatever tools are used by postscripts, ... (bash, gdb, ...)

If something has gone catastrophically wrong then I guess the build should stop but frankly I view that as unlikely versus most tests passing but some failing due to CI flakyness

DMD's test usually aren't flaky (contrary to druntime / phobos).

.deps([buildInstrumentedDmd, testRunner])
.commandFunction({
// Run dmd test suite to get data
const scope cmd = [ testRunner.targets[0], "compilable", "-j" ~ jobs.to!string ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why restrict this to compilable tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

So it runs in less than 20 years. Why would you run the tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

More balanced coverage data, especially for the backend (quite a few compilable tests use -o-). Running runnable compilable (and maybe fail_compilation) is fast when omitting the useless permutations for -O -inline ... (disabled by passing ARGS= to run.d)

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't seem to make any difference in my testing so far.

src/build.d Outdated
alias finalDataMerge = methodInit!(BuildRule, (rundBuilder, rundRule) => rundBuilder
.msg("PGO data has been merged")
.sources([ testDir.buildPath( "run.d") ])
.deps([/* genData */ genData])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.deps([/* genData */ genData])
.deps([ genData, genPhobosData ])

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet

src/build.d Outdated Show resolved Hide resolved
src/build.d Show resolved Hide resolved
src/build.d Outdated Show resolved Hide resolved
src/build.d Outdated Show resolved Hide resolved
@@ -96,6 +98,7 @@ Examples
./build.d unittest # runs internal unittests
./build.d clean # remove all generated files
./build.d generated/linux/release/64/dmd.conf
./build.d dmd-pgo # builds dmd with PGO data, currently only LDC is supported
Copy link
Contributor

@nordlow nordlow Mar 10, 2022

Choose a reason for hiding this comment

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

Why isn't this called release-pgo or pgo-release? Ahh, because it was previously called dmd.

@maxhaton maxhaton force-pushed the amongus branch 3 times, most recently from bce471c to 5334dd5 Compare March 27, 2022 22:50
@maxhaton
Copy link
Member Author

@dkorpel @RazvanN7 The CircleCI failure seems to be due to CircleCI rather than this PR (it fails with or without any actual changes in the branch)

This introduces a new target to build.d, dmd-pgo, which will build a dmd with PGO
instrumentation, then run either the phobos or dmd testsuite (both are present in the
source code, only the dmd one is set to run at the moment). This data is then merged,
at which point the data is used to build a release + LTO build of dmd.

This resulting build is significantly faster - on the order of 45% on some programs.

Making the release scripts use the PGO build may require further changes.
@ibuclaw
Copy link
Member

ibuclaw commented Mar 27, 2022

CircleCI passes #13900 🤷

@thewilsonator
Copy link
Contributor

Superseded by #13900

@maxhaton maxhaton deleted the amongus branch March 28, 2022 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants