Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-101525: Fix make test if the --enable-bolt enabled#103574

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
corona10 merged 1 commit intopython:mainfromcorona10:gh-101525-test
Apr 17, 2023

Conversation

@corona10
Copy link
Member

@corona10corona10 commentedApr 16, 2023
edited by bedevere-bot
Loading

Themake test failed without this change due to the already BOLTed binary.
Skip the BOLTrized process if the binary is already BOLTed.

$ make testRebuilding with profile guided optimizations:rm -f profile-clean-stampmake build_all CFLAGS_NODIST=" -fprofile-use -fprofile-correction" LDFLAGS_NODIST=""make[1]: Entering directory '/home1/corona10/cpython'./python -E -S -m sysconfig --generate-posix-vars ;\if test $? -ne 0 ; then \        echo "generate-posix-vars failed" ; \        rm -f ./pybuilddir.txt ; \        exit 1 ; \fi./python -E -c 'import sys ; from sysconfig import get_platform ; print("%s-%d.%d" % (get_platform(), *sys.version_info[:2]))' >platformThe necessary bits to build these optional modules were not found:nisTo find the necessary bits, look in configure.ac and config.log.Checked 111 modules (30 built-in, 79 shared, 1 n/a on linux-x86_64, 0 disabled, 1 missing, 0 failed on import)make[1]: Leaving directory '/home1/corona10/cpython'rm -f *.fdata/usr/local/bin/llvm-bolt ./python -instrument -instrumentation-file-append-pid -instrumentation-file=/home1/corona10/cpython/python.bolt -o python.bolt_instBOLT-INFO: Target architecture: x86_64BOLT-INFO: BOLT version: 712dfec1781db8aa92782b98cac5517db548b7f9/usr/local/bin/llvm-bolt: './python': BOLT-ERROR: input file was processed by BOLT. Cannot re-optimize.make: *** [Makefile:827: bolt-opt] Error 1

I used the recommended approach to detect the BOLTed binary from the BOLT team.
llvm/llvm-project#60253

Copy link
Contributor

@erlend-aaslanderlend-aasland left a comment

Choose a reason for hiding this comment

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

AC changes look good to me. For the Makefile changes, I trust you know what you are doing, because I don't :)

corona10 reacted with heart emoji
@corona10
Copy link
MemberAuthor

AC changes look good to me. For the Makefile changes, I trust you know what you are doing, because I don't :)

Look like I got approval from the Erlend at SLC :)

erlend-aasland reacted with laugh emoji

indygreg added a commit to indygreg/cpython that referenced this pull requestMay 16, 2023
This commit overhauls the make-based build system's rules forbuilding optimized binaries. Along the way it fixes a myriad ofbugs and shortcomings with the prior approach.The old way of producing optimized binaries had various limitations:* `make [all]` would do work when PGO was enabled because the phony  `profile-opt` rule was non-empty. This prevented no-op PGO builds   from working at all. This meant workflows like `make; make install`   either incurred extra work or failed due to race conditions.* Same thing for BOLT, as its `bolt-opt` rule was also non-empty  and always ran during `make [all]`.* BOLT could not be run multiple times without a full rebuild because  `llvm-bolt` can't instrument binaries that have already received  BOLT optimizations.* It was difficult to run BOLT on its own because of how various make  targets and their dependencies were structured.* I found the old way that configure and make communicated the default  targets to be confusing and hard to understand.There are essentially 2 major changes going on in this commit:1. A rework of the high-level make targets for performing a build and   how they are defined.2. A rework of all the make logic related to profile-based optimization   (read: PGO and BOLT).Build Target Rework===================Before, we essentially had `build_all`, `profile-opt`, `bolt-opt` and`build_wasm` as our 3 targets for performing a build. `all` would aliasto one of these, as appropriate.And there was another definition for which _simple_ make target toevaluate for non-optimized builds. This was likely `build_all` or`all`.In the rework, we introduce 2 new high-level targets:* `build-plain` - Perform a build without optimizations.* `build-optimized` - Perform a build with optimizations.`build-plain` is aliased to `build_all` in all configurations exceptWASM, where it is `build_wasm`.`build-optimized` by default is aliased to a target that prints an errormessage when optimizations aren't enabled. If PGO or BOLT are enabled,it is aliased to their respective target.`build-optimized` is the logical successor to `profile-opt`.I felt it best to delete `profile-opt` completely, as the new `build-*`high-level targets feel more friendly to use. But if people lament itsloss, we can add a `profile-opt: build-optimized` to achieve almost thesame result.Profiled-Based Optimization Rework==================================Most of the make logic related to profile-based optimization (read: PGOand BOLT) has been touched in this change.A major issue with the old way of doing things was we used phony,always-executed make rules. This is a bad practice in make because itundermines no-op builds.Another issue is that the separation between the rules and what orderthey ran in wasn't always clear. Both PGO and BOLT consist of the same4 phase solution: instrument, run, analyze, and apply. However, thesesteps weren't clearly expressed in the make logic. This is especiallytrue for BOLT, which only had 1 make rule.Another issue with BOLT is that it was really easy to get things intoa bad state. e.g. if you applied BOLT to `pythonX.Y` you could notrun BOLT again unless you rebuilt `pythonX.Y` from source.In the new world, we have separate `profile-<tool>-<stage>-stamp`rules defining the 4 distinct `instrument`, `run`, `analyze`, and`apply` stages for both PGO and BOLT. Each of these stages is trackedby a _stamp_ semaphore file so progress can be captured. This shouldall be pretty straightforward.There is some minimal complexity here to handle BOLT's optionaldependency on PGO, as BOLT either depends on `build_all` or`profile-pgo-apply-stamp`.As part of the refactor to BOLT we also preserve the original inputbinary before BOLT is applied. This original file is restored ifBOLT runs again. This greatly simplifies repeated BOLT invocations,as make doesn't perform needless work. However, this is all besteffort, as it is possible for some make target evaluations to stillget things in a bad state.Other Remarks=============This change effectively revertspythongh-103574. The readelf based mechanisminserted by that change was effectively working around shortcomingsin the make DAG. This change addresses those shortcomings so thereadelf integration is no longer required.If this change perturbs any bugs, they are likely around cleaningbehavior. The cleaning rules are a bit complicated and not clearlydocumented. And I'm unsure which targets CPython developers ofteniterate on. It is highly possible that state cleanup of PGO and/orBOLT files isn't as robust as it needs to be.I explicitly deleted some calls to PGO cleanup because those callsprevented no-op `make [all]` from working. It is certainly possiblesomething somewhere (release automation?) relied on these files beingdeleted when they no longer are. We still have targets to purge profilefiles and it should be trivial to add these to appropriate make rules.What I'm trying to say is there are likely subtle workflow regressionswith this refactor. But in my mind it is 3 steps forward 1 step back.It should be pretty straightforward to fix any regressions once peoplecomplain.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@corona10@erlend-aasland@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp