[clang][cmake] Fixes for PGO builds when invoking ninja twice

[tstellar]

This fixes two problems with the 2-stage PGO builds. The first problem was that the stage2-instrumented and stage2 targets would not be built on the second ninja invocation. For example:

This would work as expected.
$ ninja -v -C build stage2-instrumented-generate-profdata

Edit a file.
$ touch llvm/lib/Support/StringExtras.cpp

This would rebuild stage1 only and not build stage2-instrumented or
regenerate the profile data.
$ ninja -v -C build stage2-instrumented-generate-profdata

The second problem was that in some cases, the profile data would be regenerated, but not all of the stage2 targets would be rebuilt. One common scenario where this would happen is:

This would work as expected.
$ ninja -C build stage2-check-all

This would regenerate the profile data, but then only build the
targets that were needed for install, but weren't needed for
check-all. This would cause errors like:
ld.lld: error: Function Import: link error: linking module flags
'ProfileSummary': IDs have conflicting values in ...
This is because the executibles being built for the install target
used the new profile data, but they were linking with libraries that
used the old profile data.
$ ninja -C build stage2-install

With this change we can re-enable PGO for the release builds.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Tom Stellard (tstellar)

Changes

This fixes two problems with the 2-stage PGO builds. The first problem was that the stage2-instrumented and stage2 targets would not be built on the second ninja invocation. For example:

This would work as expected.

$ ninja -v -C build stage2-instrumented-generate-profdata

Edit a file.

$ touch llvm/lib/Support/StringExtras.cpp

This would rebuild stage1 only and not build stage2-instrumented or

regenerate the profile data.

$ ninja -v -C build stage2-instrumented-generate-profdata

The second problem was that in some cases, the profile data would be regenerated, but not all of the stage2 targets would be rebuilt. One common scenario where this would happen is:

This would work as expected.

$ ninja -C build stage2-check-all

This would regenerate the profile data, but then only build the

targets that were needed for install, but weren't needed for

check-all. This would cause errors like:

ld.lld: error: Function Import: link error: linking module flags

'ProfileSummary': IDs have conflicting values in ...

This is because the executibles being built for the install target

used the new profile data, but they were linking with libraries that

used the old profile data.

$ ninja -C build stage2-install

With this change we can re-enable PGO for the release builds.

---

Full diff: https://github.com/llvm/llvm-project/pull/92591.diff

3 Files Affected:

• (modified) clang/CMakeLists.txt (+6-12)
• (modified) clang/cmake/caches/Release.cmake (+1-1)
• (modified) clang/utils/perf-training/CMakeLists.txt (+14-3)

diff --git a/clang/CMakeLists.txt b/clang/CMakeLists.txt
index c20ce47a12abb..b4cf83d18c4e6 100644
--- a/clang/CMakeLists.txt
+++ b/clang/CMakeLists.txt
@@ -849,23 +849,17 @@ if (CLANG_ENABLE_BOOTSTRAP)
     set(CLANG_BOOTSTRAP_TARGETS check-llvm check-clang check-all)
   endif()
   foreach(target ${CLANG_BOOTSTRAP_TARGETS})
-    # Install targets have side effects, so we always want to execute them.
-    # "install" is reserved by CMake and can't be used as a step name for
-    # ExternalProject_Add_Step, so we can match against "^install-" instead of
-    # "^install" to get a tighter match. CMake's installation scripts already
-    # skip up-to-date files, so there's no behavior change if you install to the
-    # same destination multiple times.
-    if(target MATCHES "^install-")
-      set(step_always ON)
-    else()
-      set(step_always OFF)
-    endif()
 
     ExternalProject_Add_Step(${NEXT_CLANG_STAGE} ${target}
       COMMAND ${CMAKE_COMMAND} --build <BINARY_DIR> --target ${target}
       COMMENT "Performing ${target} for '${NEXT_CLANG_STAGE}'"
       DEPENDEES configure
-      ALWAYS ${step_always}
+      # We need to set ALWAYS to ON here, otherwise these targets won't be
+      # built on a second invocation of ninja.  The targets have their own
+      # logic to determine if they should build or not so setting ALWAYS ON
+      # here does not mean the targets will always rebuild it just means that
+      # they will check their dependenices and see if they need to be built.
+      ALWAYS ON
       EXCLUDE_FROM_MAIN ON
       USES_TERMINAL 1
     )
diff --git a/clang/cmake/caches/Release.cmake b/clang/cmake/caches/Release.cmake
index c0bfcbdfc1c2a..d40a84cf2a5f0 100644
--- a/clang/cmake/caches/Release.cmake
+++ b/clang/cmake/caches/Release.cmake
@@ -30,7 +30,7 @@ endfunction()
 #
 # cmake -D LLVM_RELEASE_ENABLE_PGO=ON -C Release.cmake
 set(LLVM_RELEASE_ENABLE_LTO THIN CACHE STRING "")
-set(LLVM_RELEASE_ENABLE_PGO OFF CACHE BOOL "")
+set(LLVM_RELEASE_ENABLE_PGO ON CACHE BOOL "")
 set(LLVM_RELEASE_ENABLE_RUNTIMES "compiler-rt;libcxx;libcxxabi;libunwind" CACHE STRING "")
 set(LLVM_RELEASE_ENABLE_PROJECTS "clang;lld;lldb;clang-tools-extra;bolt;polly;mlir;flang" CACHE STRING "")
 # Note we don't need to add install here, since it is one of the pre-defined
diff --git a/clang/utils/perf-training/CMakeLists.txt b/clang/utils/perf-training/CMakeLists.txt
index 61df987da7139..49673790ff6e8 100644
--- a/clang/utils/perf-training/CMakeLists.txt
+++ b/clang/utils/perf-training/CMakeLists.txt
@@ -15,7 +15,7 @@ if(LLVM_BUILD_INSTRUMENTED)
   add_lit_testsuite(generate-profraw "Generating clang PGO data"
     ${CMAKE_CURRENT_BINARY_DIR}/pgo-data/
     EXCLUDE_FROM_CHECK_ALL
-    DEPENDS clang clear-profraw ${CLANG_PGO_TRAINING_DEPS}
+    DEPENDS clear-profraw
     )
 
   add_custom_target(clear-profraw
@@ -29,10 +29,21 @@ if(LLVM_BUILD_INSTRUMENTED)
   if(NOT LLVM_PROFDATA)
     message(STATUS "To enable merging PGO data LLVM_PROFDATA has to point to llvm-profdata")
   else()
-    add_custom_target(generate-profdata
+    add_custom_command(
+      OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/clang.profdata
+      # generate-profraw is a custom_target which are always considered stale.
+      # If we add it here to 'DEPENDS', then it will always execute and running
+      # ninja install && ninja check-all will result in the profile data being
+      # generated twice, and cause the ninja check-all build to fail with errors like:
+      # `ld.lld: error: Function Import: link error: linking module flags 'ProfileSummary': IDs have conflicting values in`
+      # Therefor we call the generate-profraw target manually as part of this custom
+      # command, which will only run if clang or ${CLANG_PGO_TRAINING_DEPS} are updated.
+      COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} --target generate-profraw
       COMMAND "${Python3_EXECUTABLE}" ${CMAKE_CURRENT_SOURCE_DIR}/perf-helper.py merge ${LLVM_PROFDATA} ${CMAKE_CURRENT_BINARY_DIR}/clang.profdata ${CMAKE_CURRENT_BINARY_DIR} ${CMAKE_BINARY_DIR}/profiles/
       COMMENT "Merging profdata"
-      DEPENDS generate-profraw)
+      DEPENDS clang ${CLANG_PGO_TRAINING_DEPS}
+    )
+    add_custom_target(generate-profdata DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/clang.profdata)
     if (CLANG_PGO_TRAINING_DATA_SOURCE_DIR)
       llvm_ExternalProject_Add(generate-profraw-external ${CLANG_PGO_TRAINING_DATA_SOURCE_DIR}
               USE_TOOLCHAIN EXLUDE_FROM_ALL NO_INSTALL DEPENDS generate-profraw)

[tstellar]

Ping.

[llvm-beanz]

LGTM. Sorry for the delay.