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

Show file tree
Hide file tree
Changes from 18 commits
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
1 change: 1 addition & 0 deletions Misc/ACKS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,7 @@ Grant Edwards
Vlad Efanov
Zvi Effron
John Ehresman
Chris Eibl
Tal Einat
Eric Eisner
Andrew Eland
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Build/2025-02-13-19-21-41.gh-issue-130090.3ngJaV.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Building with ``PlatformToolset=ClangCL`` on Windows now supports PGO
(profile guided optimization). Patch by Chris Eibl.
1 change: 1 addition & 0 deletions PCbuild/_freeze_module.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@
<AdditionalIncludeDirectories>$(IntDir);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
<Optimization>Disabled</Optimization>
<WholeProgramOptimization>false</WholeProgramOptimization>
<AdditionalOptions Condition="$(PlatformToolset) == 'ClangCL'">%(AdditionalOptions) -fno-lto</AdditionalOptions>
</ClCompile>
<Link>
<SubSystem>Console</SubSystem>
Expand Down
48 changes: 48 additions & 0 deletions PCbuild/pyproject-clangcl.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<PropertyGroup Label="Globals">
<__PyprojectClangCl_Props_Imported>true</__PyprojectClangCl_Props_Imported>
</PropertyGroup>

<PropertyGroup>
<!-- CLANG_PROFILE_PATH is configurable for "remote PGO builds"
For convenience, we also accept paths without trailing slashes.
-->
<CLANG_PROFILE_PATH Condition="'$(CLANG_PROFILE_PATH)' == ''">$(OutDir)</CLANG_PROFILE_PATH>
<_CLANG_PROFILE_PATH>$(CLANG_PROFILE_PATH)</_CLANG_PROFILE_PATH>
<_CLANG_PROFILE_PATH Condition="!HasTrailingSlash($(_CLANG_PROFILE_PATH))">$(_CLANG_PROFILE_PATH)\</_CLANG_PROFILE_PATH>
</PropertyGroup>

<ItemGroup>
<_profrawFiles Include="$(OutDir)instrumented\$(TargetName)_*.profraw" />
</ItemGroup>

<Target Name="EnsureClangProfileData" BeforeTargets="PrepareForBuild"
Condition="'$(SupportPGO)' and $(Configuration) == 'PGUpdate'">
<Error Text="PGO run did not succeed (no $(TargetName)_*.profraw files) and there is no data to merge"
Condition="$(RequireProfileData) == 'true' and @(_profrawFiles) == ''" />
</Target>

<Target Name="MergeClangProfileData" BeforeTargets="PrepareForBuild"
Condition="'$(SupportPGO)' and $(Configuration) == 'PGUpdate'"
Inputs="@(_profrawFiles)"
Outputs="$(OutDir)instrumented\profdata.profdata">
<Exec
Command='"$(LLVMInstallDir)\bin\llvm-profdata.exe" merge -output="$(OutDir)instrumented\profdata.profdata" "$(OutDir)instrumented\*_*.profraw"' />
</Target>

<Target Name="CleanClangProfileData" BeforeTargets="Clean">
<Delete Files="@(_profrawFiles)" TreatErrorsAsWarnings="true" />
<Delete Files="$(OutDir)instrumented\profdata.profdata" TreatErrorsAsWarnings="true" />
</Target>

<ItemDefinitionGroup>
<ClCompile>
<AdditionalOptions>-Wno-deprecated-non-prototype -Wno-unused-label -Wno-pointer-sign -Wno-incompatible-pointer-types-discards-qualifiers -Wno-unused-function %(AdditionalOptions)</AdditionalOptions>
<AdditionalOptions Condition="$(Configuration) != 'Debug'">-flto %(AdditionalOptions)</AdditionalOptions>
<AdditionalOptions Condition="$(SupportPGO) and $(Configuration) == 'PGInstrument'">-fprofile-instr-generate=$(_CLANG_PROFILE_PATH)$(TargetName)_%m.profraw %(AdditionalOptions)</AdditionalOptions>
<AdditionalOptions Condition="$(SupportPGO) and $(Configuration) == 'PGUpdate'">-fprofile-instr-use=$(OutDir)instrumented\profdata.profdata -Wno-profile-instr-unprofiled %(AdditionalOptions)</AdditionalOptions>
</ClCompile>
</ItemDefinitionGroup>

</Project>
9 changes: 5 additions & 4 deletions PCbuild/pyproject.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
<LinkIncremental Condition="$(Configuration) != 'Debug'">false</LinkIncremental>
</PropertyGroup>

<!-- We need the above overridden OutDir, so this must be imported after PropertyGroup -->
<Import Project="pyproject-clangcl.props" Condition="$(PlatformToolset) == 'ClangCL' and $(__PyprojectClangCl_Props_Imported) != 'true'" />

<PropertyGroup Condition="$(TargetExt) != ''">
<TargetNameExt>$(TargetName)$(TargetExt)</TargetNameExt>
<_TargetNameSep>$(TargetNameExt.LastIndexOf(`.`))</_TargetNameSep>
Expand Down Expand Up @@ -69,8 +72,6 @@
<ControlFlowGuard Condition="$(EnableControlFlowGuard) != ''">$(EnableControlFlowGuard)</ControlFlowGuard>
<MultiProcessorCompilation>true</MultiProcessorCompilation>
<AdditionalOptions>/utf-8 %(AdditionalOptions)</AdditionalOptions>
<AdditionalOptions Condition="$(PlatformToolset) == 'ClangCL'">-Wno-deprecated-non-prototype -Wno-unused-label -Wno-pointer-sign -Wno-incompatible-pointer-types-discards-qualifiers -Wno-unused-function %(AdditionalOptions)</AdditionalOptions>
<AdditionalOptions Condition="$(Configuration) != 'Debug' and $(PlatformToolset) == 'ClangCL'">-flto %(AdditionalOptions)</AdditionalOptions>
<AdditionalOptions Condition="$(MSVCHasBrokenARM64Clamping) == 'true' and $(Platform) == 'ARM64'">-d2pattern-opt-disable:-932189325 %(AdditionalOptions)</AdditionalOptions>
<AdditionalOptions Condition="$(MSVCHasBrokenARM64SignExtension) == 'true' and $(Platform) == 'ARM64'">-d2ssa-patterns-all- %(AdditionalOptions)</AdditionalOptions>
<AdditionalOptions Condition="$(GenerateSourceDependencies) == 'true'">/sourceDependencies "$(IntDir.Trim(`\`))" %(AdditionalOptions)</AdditionalOptions>
Expand Down Expand Up @@ -186,15 +187,15 @@ public override bool Execute() {
Targets="CleanAll" />
</Target>

<Target Name="CopyPGCFiles" BeforeTargets="PrepareForBuild" Condition="$(Configuration) == 'PGUpdate'">
<Target Name="CopyPGCFiles" BeforeTargets="PrepareForBuild" Condition="$(Configuration) == 'PGUpdate' and $(PlatformToolset) != 'ClangCL'">
<ItemGroup>
<_PGCFiles Include="$(OutDir)instrumented\$(TargetName)!*.pgc" />
<_PGDFile Include="$(OutDir)instrumented\$(TargetName).pgd" />
<_CopyFiles Include="@(_PGCFiles);@(_PGDFile)" Condition="Exists(%(FullPath))" />
</ItemGroup>
<Delete Files="@(_CopyFiles->'$(OutDir)%(Filename)%(Extension)')" />
<Error Text="PGO run did not succeed (no $(TargetName)!*.pgc files) and there is no data to merge"
Condition="$(RequirePGCFiles) == 'true' and @(_PGCFiles) == ''" />
Condition="$(RequireProfileData) == 'true' and @(_PGCFiles) == ''" />
<Copy SourceFiles="@(_CopyFiles)"
DestinationFolder="$(OutDir)"
UseHardLinksIfPossible="true"
Expand Down
2 changes: 1 addition & 1 deletion PCbuild/pythoncore.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@
</ImportGroup>
<PropertyGroup>
<KillPython>true</KillPython>
<RequirePGCFiles>true</RequirePGCFiles>
<RequireProfileData>true</RequireProfileData>
<IncludeExternals Condition="$(IncludeExternals) == '' and Exists('$(zlibDir)\zlib.h')">true</IncludeExternals>
<IncludeExternals Condition="$(IncludeExternals) == ''">false</IncludeExternals>
</PropertyGroup>
Expand Down
36 changes: 36 additions & 0 deletions PCbuild/readme.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,42 @@ Release
settings, though without PGO.


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.
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
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).


You can also use a specific version of clang-cl downloaded from
https://github.com/llvm/llvm-project/releases, e.g.
clang+llvm-18.1.8-x86_64-pc-windows-msvc.tar.xz.
Given you have extracted that to <my-clang-dir>, you can use it like so
build.bat --pgo "/p:PlatformToolset=ClangCL" "/p:LLVMInstallDir=<my-clang-dir> "/p:LLVMToolsVersion=18"

Here, PreferredToolArchitecture is not needed, because this is a 64bit
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
"/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.
Comment on lines +82 to +84
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs a worked example. Just from this description, I couldn't confidently specify the parameter and trust that I got it right.

Like in the MSVC case, after fetching (or manually copying) the instrumented
folder back into your build tree, you can continue with the PGUpdate
step with no further parameters.

Building Python using the build.bat script
----------------------------------------------

Expand Down
Loading