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 3 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 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
3 changes: 2 additions & 1 deletion PCbuild/pcbuild.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@
</ItemGroup>

<Target Name="Build">
<MSBuild Projects="@(FreezeProjects)"
<MSBuild Condition="$(Configuration) != 'PGUpdate'"
Projects="@(FreezeProjects)"
Properties="Configuration=%(Configuration);Platform=%(Platform);%(Properties)"
BuildInParallel="%(BuildInParallel)"
StopOnFirstFailure="true"
Expand Down
6 changes: 5 additions & 1 deletion PCbuild/pyproject.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
<Py_IntDir Condition="'$(Py_IntDir)' == ''">$(MSBuildThisFileDirectory)obj\</Py_IntDir>
<IntDir>$(Py_IntDir)\$(MajorVersionNumber)$(MinorVersionNumber)$(ArchName)_$(Configuration)\$(ProjectName)\</IntDir>
<IntDir>$(IntDir.Replace(`\\`, `\`))</IntDir>
<ClangProfileDir>$(Py_IntDir)\$(MajorVersionNumber)$(MinorVersionNumber)$(ArchName)_PGInstrument\__clang_profiles\</ClangProfileDir>
<ClangProfileDir>$(ClangProfileDir.Replace(`\\`, `\`))</ClangProfileDir>
<!-- pyconfig.h is updated by pythoncore.vcxproj, so it's always in pythoncore's IntDir -->
<GeneratedPyConfigDir>$(Py_IntDir)\$(MajorVersionNumber)$(MinorVersionNumber)$(ArchName)_$(Configuration)\pythoncore\</GeneratedPyConfigDir>
<GeneratedFrozenModulesDir>$(Py_IntDir)\$(MajorVersionNumber)$(MinorVersionNumber)_frozen\</GeneratedFrozenModulesDir>
Expand Down Expand Up @@ -70,6 +72,8 @@
<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="$(SupportPGO) and $(Configuration) == 'PGInstrument' and $(PlatformToolset) == 'ClangCL'">-fprofile-instr-generate=$(ClangProfileDir)\default_%m.profraw %(AdditionalOptions)</AdditionalOptions>
<AdditionalOptions Condition="$(SupportPGO) and $(Configuration) == 'PGUpdate' and $(PlatformToolset) == 'ClangCL'">-fprofile-instr-use=$(ClangProfileDir)\profdata.profdata %(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 @@ -185,7 +189,7 @@ 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" />
Expand Down
13 changes: 13 additions & 0 deletions PCbuild/pythoncore.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,7 @@
<ClCompile Include="..\Modules\blake2module.c">
<PreprocessorDefinitions Condition="'$(Platform)' == 'x64'">HACL_CAN_COMPILE_SIMD128;%(PreprocessorDefinitions)</PreprocessorDefinitions>
<PreprocessorDefinitions Condition="'$(Platform)' == 'x64'">HACL_CAN_COMPILE_SIMD256;%(PreprocessorDefinitions)</PreprocessorDefinitions>
<AdditionalOptions>/arch:AVX</AdditionalOptions>
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed right? It's not always true.

Copy link
Member Author

Choose a reason for hiding this comment

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

My clang-cl builds error without that and it does not hurt MSVC AFACT.
It is also used for Hacl_Hash_Blake2b_Simd256.c and Hacl_Hash_Blake2s_Simd128.c, but there with Condition="'$(Platform)' == 'x64'.

I think, I should add that condition here, too.

If in doubt about MSVC, I could also furthermore add Condition="$(PlatformToolset) == 'ClangCL'.

</ClCompile>
<ClCompile Include="..\Modules\_codecsmodule.c" />
<ClCompile Include="..\Modules\_collectionsmodule.c" />
Expand Down Expand Up @@ -716,6 +717,18 @@
<Delete Files="$(IntDir)pyconfig.h;$(OutDir)pyconfig.h" />
</Target>

<Target Name="EnsureClangProfileDir" BeforeTargets="PrepareForBuild"
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 think, we should not only ensure the profile dir, but empty it.

But most probably only when a full rebuild / clean is done.

Condition="$(PlatformToolset) == 'ClangCL' and $(Configuration) == 'PGInstrument'">
<MakeDir Directories="$(ClangProfileDir)" Condition="!Exists($(ClangProfileDir))" />
</Target>

<Target Name="MergeClangProfileData" BeforeTargets="PrepareForBuild"
Condition="$(PlatformToolset) == 'ClangCL' and $(Configuration) == 'PGUpdate'">
<Message Importance="high" Text="Merging clang profile data in $(ClangProfileDir)" />
<Exec
Command='"$(LLVMInstallDir)\bin\llvm-profdata.exe" merge -output=$(ClangProfileDir)\profdata.profdata $(ClangProfileDir)\default_*.profraw' />
</Target>

<Target Name="_GetBuildInfo" BeforeTargets="PrepareForBuild">
<PropertyGroup>
<GIT Condition="$(GIT) == ''">git</GIT>
Expand Down
Loading