[BOLT][CMake] Redo the build and install targets

[phosek]

Summary

The existing BOLT install targets are broken on Windows because they
don't properly handle the output extension. We cannot use the existing
LLVM macros since those make assumptions that don't hold for BOLT. This
change instead implements custom macros following the approach used by
Clang and LLD.

[phosek]

Ping, our toolchain build on Windows is currently broken because of BOLT and I verified that this change resolves the issue so I'd appreciate a review.

[smeenai]

LGTM. Sorry for the delay, I was out for Memorial Day weekend.

[phosek]

I had to redo the patch after investigating the builder failure and better understanding the intention of the original code.

[phosek]

There's still an issue where only bolt is included in LLVM_DISTRIBUTION_COMPONENTS and not individual tools such as llvm-bolt because then the install targets won't be created because of https://github.com/llvm/llvm-project/blob/c7eb1b07470b9babfcd258f014df3661e5f84b30/llvm/cmake/modules/AddLLVM.cmake#L1348. Unfortunately this scenario where the name of the component is different from the tool itself is currently supported in the LLVM build.

[phosek]

@smeenai Do you have any thoughts on this? I don't actually know how to fix this. The intention seems to be allow the use of the bolt target/component as a grouping for other BOLT targets such as llvm-bolt or bolt_rt, so you can only include bolt in LLVM_DISTRIBUTION_COMPONENT. I understand why that may be desirable, but that breaks many of the assumptions that existing macros make and I don't know how to easily fix it.

[smeenai]

Hmm, could we extend the existing umbrella target support for things like llvm-libraries and clang-libraries to cover the BOLT case as well?

bolt/tools/driver/CMakeLists.txt

47

I assume this should be add_dependencies?

50

This should depend on the stripped target?

[phosek]

In D151595#4382652, @smeenai wrote:

Hmm, could we extend the existing umbrella target support for things like llvm-libraries and clang-libraries to cover the BOLT case as well?

I have opted for an approach that's similar to the one used by Clang and LLD, although the handling of components is different to preserve the existing behavior. This seems to be working in my local testing.

[smeenai]

Makes sense, though @Amir should also take a look.

bolt/CMakeLists.txt

122 ↗

(On Diff #526948)

Where is BOLT_DEPENDS being set now?

The CMake docs say DEPENDS should only be used for add_custom_command outputs in the same folder, and add_dependencies for everything else, so we want the latter here. I know this is just moving existing code, but we may as well fix that while we're here. I don't think it makes any difference for Ninja, but it might for other generators?

bolt/tools/heatmap/CMakeLists.txt

8 ↗

(On Diff #526948)

Why this change?

[phosek]

@Amir does this change look good to you?

bolt/tools/heatmap/CMakeLists.txt

8 ↗

(On Diff #526948)

This tool wasn't previously included in the bolt umbrella target so I assume that it doesn't need an install target, but I'd be helpful for @Amir to confirm.

[Amir]

In D151595#4385506, @phosek wrote:

@Amir does this change look good to you?

Looks good overall. Thanks for adding AddBolt cmake module. Testing the build internally, will reply shortly.

[Amir]

bolt/test/CMakeLists.txt

40 ↗

(On Diff #527084)

We have a number of dependencies on llvm-boltdiff and perf2bolt, e.g. in internal and upstream binary tests: https://github.com/rafaelauler/bolt-tests/blob/main/CMakeLists.txt#L22. Can we keep these targets somehow?

bolt/tools/heatmap/CMakeLists.txt

8 ↗

(On Diff #526948)

It wasn't included by omission. Let's include it into the umbrella target.

[phosek]

bolt/test/CMakeLists.txt

40 ↗

(On Diff #527084)

These symlinks are now always generated, see the ALWAYS_GENERATED option in add_bolt_tool_symlink so there's no need to specify those as targets here separately.

[Amir]

Thanks. Will update the dependencies, removing always-installed symlinks.

[Amir]

Merged the PR. Please reland

bolt/tools/CMakeLists.txt

 set(BOLT_TOOLS_INSTALL_DIR "${CMAKE_INSTALL_BINDIR}" CACHE PATH
     "Path for binary subdirectory (defaults to '${CMAKE_INSTALL_BINDIR}')")
 mark_as_advanced(BOLT_TOOLS_INSTALL_DIR)
 
 # Move these macros to AddBolt if such a CMake module is ever created.
 
 macro(add_bolt_tool name)
   llvm_add_tool(BOLT ${ARGV})
+  install(TARGETS ${name}
+    DESTINATION ${CMAKE_INSTALL_BINDIR}
+    COMPONENT ${name}
+    )
+  add_llvm_install_targets(install-${name} DEPENDS ${name} COMPONENT ${name})
 endmacro()
 
-macro(add_bolt_tool_symlink name)
-  llvm_add_tool_symlink(BOLT ${ARGV})
+macro(add_bolt_tool_symlink name dest)
+  llvm_add_tool_symlink(BOLT ${name} ${dest})
+  llvm_install_symlink(BOLT ${name} ${dest} ALWAYS_GENERATE)
 endmacro()
 
 add_subdirectory(driver)
 add_subdirectory(llvm-bolt-fuzzer)
 add_subdirectory(bat-dump)
 add_subdirectory(merge-fdata)
 add_subdirectory(heatmap)

bolt/tools/driver/CMakeLists.txt

 
 set(BOLT_DEPENDS
   llvm-bolt
   perf2bolt
   llvm-boltdiff
   )
 
 add_custom_target(bolt DEPENDS ${BOLT_DEPENDS})
-install(PROGRAMS
-  ${CMAKE_BINARY_DIR}/bin/llvm-bolt
-  ${CMAKE_BINARY_DIR}/bin/perf2bolt
-  ${CMAKE_BINARY_DIR}/bin/llvm-boltdiff
-  DESTINATION ${CMAKE_INSTALL_BINDIR}
-  COMPONENT bolt
-  )
 add_llvm_install_targets(install-bolt DEPENDS bolt COMPONENT bolt)
 set_target_properties(bolt PROPERTIES FOLDER "BOLT")
 set_target_properties(install-bolt PROPERTIES FOLDER "BOLT")
 
 include_directories( ${BOLT_SOURCE_DIR}/lib )

bolt/tools/merge-fdata/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS Support)
 
 add_bolt_tool(merge-fdata
   merge-fdata.cpp
 
   DISABLE_LLVM_LINK_LLVM_DYLIB
 
   DEPENDS
   intrinsics_gen
 )
 set_target_properties(merge-fdata PROPERTIES FOLDER "BOLT")
 
 add_dependencies(bolt merge-fdata)
-install(PROGRAMS
-  ${CMAKE_BINARY_DIR}/bin/merge-fdata
-  DESTINATION ${CMAKE_INSTALL_BINDIR}
-  COMPONENT bolt
-  )
 
 # Emit relocations for BOLT meta test (bolt/test/runtime/meta-merge-fdata.test)
 if (BOLT_INCLUDE_TESTS AND UNIX AND NOT APPLE)
   target_link_options(merge-fdata PRIVATE LINKER:--emit-relocs)
 endif()