8000 gh-130090: Support PGO for clang-cl by chris-eibl · Pull Request #129907 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-130090: Support PGO for clang-cl #129907

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.

Sign up for GitHub

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

Merged
merged 24 commits into from
Mar 4, 2025
Merged

gh-130090: Support PGO for clang-cl #129907

Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
859c89f
minimal changes needed for PGO with clang-cl
chris-eibl Feb 9, 2025
a529c39
like for MSVC, don't use link time optimization
chris-eibl Feb 9, 2025
26fb51f
Do not build _freeze_module twice in case of PGO
chris-eibl Feb 9, 2025
6e2cb69
remove /arch:AVX for blake2module.c again
chris-eibl Feb 10, 2025
7b46aeb
Revert "Do not build _freeze_module twice in case of PGO"
chris-eibl Feb 10, 2025
ced66bd
always clean the profile directory when doing a new clang PGO build
chris-eibl Feb 10, 2025
1aae65d
Merge remote-tracking branch 'origin/main' into clang-pgo
chris-eibl Feb 11, 2025
52fd5ab
Merge branch 'main' into clang-pgo
chris-eibl Feb 13, 2025
c2ecb57
blurb it
chris-eibl Feb 13, 2025
ad72df5
Adding myself to ACKS
chris-eibl Feb 13, 2025
74ec74e
extract pyproject-clangcl.props
chris-eibl Feb 13, 2025
79ce418
Merge branch 'python:main' into clang-pgo
chris-eibl Feb 16, 2025
8c8aa79
move MergeClangProfileData to pyproject-clangcl.props
chris-eibl Feb 16, 2025
2a9da27
rename RequirePGCFiles to RequireProfileData
chris-eibl Feb 16, 2025
ea4de96
Apply suggestions from code review
chris-eibl Feb 18, 2025
3346b9d
Use -Wno-profile-instr-unprofiled in the PGUpdate case
chris-eibl Feb 18, 2025
9db1a29
introduce CLANG_PROFILE_PATH
chris-eibl Feb 19, 2025
4ad2365
update readme.txt
chris-eibl Feb 21, 2025
1977953
Apply suggestions from code review
chris-eibl Feb 24, 2025
ad2dfce
Apply suggestions from code review
chris-eibl Feb 24, 2025
6edbe07
credit zooba
chris-eibl Feb 24, 2025
81b4c1d
address review comments regarding PGO
chris-eibl Feb 24, 2025
263870d
Explicitely set the architecture based on $(Platform)
chris-eibl Feb 24, 2025
db208ae
Update PCbuild/readme.txt
chris-eibl Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Apply suggestions from code review 
Co-authored-by: Ken Jin <kenjin4096@gmail.com>
  • Loading branch information
@chris-eibl @Fidget-Spinner
chris-eibl and Fidget-Spinner authored Feb 24, 2025
commit 1977953f2beea21a49edfbb68d994c21efa523e0
8 changes: 4 additions & 4 deletions PCbuild/readme.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,14 @@ Building Python using Clang/LLVM
--------------------------------

See https://learn.microsoft.com/en-us/cpp/build/clang-support-msbuild?view=msvc-170
how to install and use clang-cl bundled with Microsoft Visual Studio.
for how to install and use clang-cl bundled with Microsoft Visual Studio.
You can use the IDE to switch to clang-cl for local development,
but because this alters the *.vcxproj files, the recommended way is
to use build.bat:

build.bat "/p:PlatformToolset=ClangCL" "/p:PreferredToolArchitecture=x64"

All other build.bat options remain to work as with MSVC, so this
All other build.bat options continue to work as with MSVC, so this
will create a 64bit release binary. PreferredToolArchitecture is needed,
because msbuild by default selects the 32bit architecture of the toolset,
which uses -m32 as the default target architecture.
Copy link
Member

Choose a reason for hiding this comment

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

If this is genuinely required, let's add a <PreferredToolArchitecture Condition="$(PROCESSOR_ARCHITECTURE) == 'AMD64'">x64</PreferredToolArchitecture> to the clangcl.props. I prefer to not override MSVC defaults for this property, but if Clang can't handle cross-compiling, then provided we know that it's been requested we may as well override defaults.

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH, I don't know why I need it when building with msbuild. I do not need it in case of the IDE or when explicitely using a custom installed toolset (most probably because I've always installed 64bit versions of those). I like your suggestion and will try it out - looks promising!

Copy link
Member Author

Choose a reason for hiding this comment

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

looks promising!

But did not work out: most probably, we're "too late" when setting that, meaning that these lines in Microsoft.Cpp.ClangCl.Common.props are already "resolved":

    <_DefaultLLVMInstallDir Condition="'$(_DefaultLLVMInstallDir)' == '' AND '$(PreferredToolArchitecture)' == 'arm64'">$(VsInstallRoot)\VC\Tools\Llvm\ARM64</_DefaultLLVMInstallDir>
    <_DefaultLLVMInstallDir Condition="'$(_DefaultLLVMInstallDir)' == '' AND '$(PreferredToolArchitecture)' == 'x64'">$(VsInstallRoot)\VC\Tools\Llvm\x64</_DefaultLLVMInstallDir>
    <_DefaultLLVMInstallDir Condition="'$(_DefaultLLVMInstallDir)' == '' AND '$(PreferredToolArchitecture)' != 'x64'">$(VsInstallRoot)\VC\Tools\Llvm</_DefaultLLVMInstallDir>
    <LLVMInstallDir Condition="'$(LLVMInstallDir)' == ''">$(_DefaultLLVMInstallDir)</LLVMInstallDir>

Anyway, using the PreferredToolArchitecture as a way to steer the architecture of the generated binaries was a bad idea. So to get a 32bit binary, one would have to use "/p:PreferredToolArchitecture=x86" :-O

clang-cl of course can cross-compile, so let's explicitely set the architecture based on $(Platform).

Expand All @@ -79,8 +79,8 @@ platform toolset, but LLVMToolsVersion has to be set accordingly.
Setting the major version is enough, although you can be specific
and use 18.1.8 in the above example, too.

Even --pgo works out of the box, except you do want to run the PGO task
on a different host than the build host. In this case you must pass
Even --pgo works out of the box. However, if you want to run the PGO task
on a different host than the build host, you must pass
"/p:CLANG_PROFILE_PATH=<relative-path-to-instrumented-dir-on-remote-host>"
in the PGInstrument step to make sure the profile data is generated
into the instrumented directory when running the PGO task.
Expand Down
Loading
0