Add PGO GENPROFILE support to coreclr and clrjit #7423
5 participants
|
@gkhanna79, PTAL |
| @@ -179,6 +179,11 @@ function(install_clr targetName) | ||
| else() | ||
| install(FILES ${strip_destination_file} DESTINATION .) | ||
| endif() | ||
| + if($ENV{PGO_GENERATE}) |
I am not happy with using env variables as config switches for cmake. We should add an option to the build.sh and pass it to the cmake using the -D option. The easiest is to add it to the __ExtraCmakeArgs in build.cmd.
| + endif(WIN32) | ||
| + | ||
| + file(TO_NATIVE_PATH | ||
| + "$ENV{__PackagesDir}/Microsoft.DotNet.OptimizationData.Coreclr/$ENV{__BuildOS}.$ENV{__BuildArch}/${ProfileFileName}" |
Please use ${CLR_CMAKE_TARGET_ARCH} instead of $ENV{__BuildArch} and
- add
"-DCLR_CMAKE_TARGET_OS=%__BuildOS%"to__ExtraCmakeArgsinbuild.cmdand then use it here instead of the$ENV{__BuildOS}. - add
"-DCLR_CMAKE_PACKAGES_DIR=%__PackagesDir%"to__ExtraCmakeArgsinbuild.cmdand then use it here instead of the$ENV{__PackagesDir}.
| + # Enable PGO only for optimized configs | ||
| + set(ConfigTypeList RELEASE RELWITHDEBINFO) | ||
| + | ||
| + foreach(ConfigType IN LISTS ConfigTypeList) |
Looping over configs makes no sense on single target cmake generators, like the Unix make. The cmake is run for a single specific configuration.
You can use generator expressions instead of looping over the configs. That would work in both cases. For example, the first set_property would be
set_property(TARGET ${TargetName} APPEND_STRING PROPERTY LINK_FLAGS $<$<OR:$<CONFIG:Release>,$<CONFIG:Relwithdebinfo>>:/LTCG /GENPROFILE>
@janvorli I originally tried this approach, but apparently, cmake does not expand generator expressions for LINK_FLAGS*. The best workaround I could find suggested using target_link_libraries instead, which kind of works, but on Windows it dumps the switches into <AdditionalDependencies> (next to all the *.lib files) instead of <AdditionalOptions>, and this to me seems wrong (although cmake "supports" that, we're relying on undocumented msbuild behavior there).
I could try using target_link_libraries anyway; or I could just target LINK_FLAGS_RELEASE (and simply ignore PGO for checked builds). I'm kind of leaning toward the latter here, but I'm open to it either way... what do you think? (@brianrob, @gkhanna79 - any additional thoughts?)
Ouch, that's unfortunate. I've already noticed that the generator expressions don't work at some places. So then please keep it the way you had it.
| @@ -233,7 +236,8 @@ if %__BuildNative% EQU 1 ( | ||
| echo %__MsgPrefix%Regenerating the Visual Studio solution | ||
| pushd "%__IntermediatesDir%" | ||
| - call "%__SourceDir%\pal\tools\gen-buildsys-win.bat" "%__ProjectDir%" %__VSVersion% %__BuildArch% %__BuildJit32% | ||
| + set __ExtraCmakeArgs="-DCLR_CMAKE_TARGET_OS=%__BuildOs%" "-DCLR_CMAKE_PACKAGES_DIR=%__PackagesDir%" "-DCLR_CMAKE_PGO_INSTRUMENT=%__PgoInstrument%" |
It seems we need to add the "-DCLR_CMAKE_TARGET_ARCH=%__BuildArch%" here. It is present in the other place.
@janvorli I believe it's being set as a default here. It's being set in the other location because host != target in the cross build scenario. Should I still set it here?
|
LGTM, thank you very much for adding that! |
|
Thanks @janvorli for the detailed review! I'll just wait for CI to make sure nothing unexpected broke, and if all is well I'll merge the PR. Cheers! |
Update the cmake build system to enable support for Profile Guided
Optimization (PGO) on Windows, and enable this feature for two target
binaries (coreclr and clrjit).
With this change, toggle between instrumented and profile-optimized
settings for target binaries by setting the
pgoinstrumentflag whencalling build.cmd or build.sh. Assume profile-optimized mode by default.
Fall back to regular non-PGO optimized builds if profile data is not
available.