Disable LTO and LLD for bootstrap builds on systems unsupported by LLD

[tbaeder]

Summary

I tried doing a stage3 build on SystemZ/s390 but ran into the unfortunate problem that LLD does not support SystemZ, so the stage3 build can't use LLD and also can't use LTO.
Implementing the logic to only use LTO/LLD on non-SystemZ system inside the 3-stage-base.cmake was not possible however since none of the variables to check are set at that point. According to https://cmake.org/cmake/help/latest/manual/cmake.1.html#options, a cache file passed to cmake -C should consist of set() calls to populate the cache.

I'm not sure if the previous if (APPLE) check worked on apple systems at all. I'm also not sure if setting any of the BOOTSTRAP_ variables inside a non-cache cmake file should happen at all.

I did test this patch on x86_64, s390, ppc64le and aarch64, but I'm not sure if SystemZ/s390 is the only problematic architecture and if there is a better way of checking whether we're on a non-lld-supported arch.

Anyway, happy to hear comments on this patch. Thanks!

[serge-sans-paille]

clang/CMakeLists.txt

647

Should you also disable LTO here?

[serge-sans-paille]

@beanz This patch looks good to me, can you confirm?

[serge-sans-paille]

@tbaeder sorry for the delay. I don't see any reason why we should keep that one in review any longer.

[tbaeder]

Just re-tested this on a SystemZ machine and found that the set() in clang/CMakeLists.txt were not actually being applied. I also made it disable LLD on SystemZ as well, as you suggested. And I added a warning when BOOTSTRAP_LLVM_ENABLE_LLD is TRUE but lld is not in LLVM_ENABLE_PROJECTS

[serge-sans-paille]

clang/CMakeLists.txt

639

I guess you're using FORCE here to change the user default, right? In that case wouldn't that make sense to warn only if it's an actual change?

[tbaeder]

Dropped the FORCE stuff since we can just set the variable directly and that shouldn't make a difference as far was I know. And the warning now only appears if either LTO or LLD are enabled.

[beanz]

I'm super sorry to be popping in so late on this. Is this really the direction we should take?

If we know SystemZ + LLD + LTO is a bad configuration, "fixing" the configuration and logging a message seems like the wrong move when the only way to get into that state is to specifically request options.

The CMake cache files are morally equivalent to a user specifying the config they want, and this patch accepts an invalid configuration and mutates it. Our CMake output is so noisy, that a user who actually wants LTO on SystemZ might never notice that they aren't getting it.

It seems to me, your intent is to make the stage3 build work for SystemZ, and that seems like something we should accomplish entirely inside the stage3 CMake caches.

[tbaeder]

In D89942#2629146, @beanz wrote:

If we know SystemZ + LLD + LTO is a bad configuration, "fixing" the configuration and logging a message seems like the wrong move when the only way to get into that state is to specifically request options.

But that's not true. The 3-stage-base.cmake sets BOOTSTRAP_LLVM_ENABLE_LTO to ON, which doesn't work on SystemZ. This is the initial problem I had. I was not interested in having LTO enabled or not, but the cache enables it and that breaks on SystemZ.

It seems to me, your intent is to make the stage3 build work for SystemZ, and that seems like something we should accomplish entirely inside the stage3 CMake caches.

I vaguely remember trying to do all this in the 3-stage-base.cmake but things like LLVM_NATIVE_ARCH etc. are not available in there, I am not sure if the previous if(APPLE) check was working either.

[beanz]

In D89942#2629184, @tbaeder wrote:

But that's not true. The 3-stage-base.cmake sets BOOTSTRAP_LLVM_ENABLE_LTO to ON, which doesn't work on SystemZ. This is the initial problem I had. I was not interested in having LTO enabled or not, but the cache enables it and that breaks on SystemZ.

Right, and my point is that the problem is 3-stage-base setting that value. The caches are effectively user presets for build configurations. If the preset is broken, we shouldn't work around that in the build system.

I vaguely remember trying to do all this in the 3-stage-base.cmake but things like LLVM_NATIVE_ARCH etc. are not available in there, I am not sure if the previous if(APPLE) check was working either.

The APPLE check definitely works. That value is set by CMake before loading cache values, and that should not be removed from the cache file.

You can dump this blurb at the top of the CMake cache to see what variables are defined:

get_cmake_property(_variableNames VARIABLES)                                    
list (SORT _variableNames)                                                      
foreach (_variableName ${_variableNames})                                       
    message(STATUS "${_variableName}=${${_variableName}}")                      
endforeach()

Between the variables CMake sets there and possibly shelling out to uname, is there really nothing you can key off?

[beanz]

If you add include(CMakeDetermineSystem) to the top of the cache file, it should initialize CMAKE_HOST_SYSTEM_PROCESSOR, which also probably gives you a reasonable value to key off.

[tbaeder]

Including CMakeDetermineSystem gives me:

loading initial cache file ../clang/cmake/caches/3-stage.cmake
CMake Error: Could not open file for write in copy operation /CMakeSystem.cmake.tmp
CMake Error: : System Error: Permission denied
CMake Error at /usr/share/cmake/Modules/CMakeDetermineSystem.cmake:185 (configure_file):
  configure_file Problem configuring file
Call Stack (most recent call first):
  /home/tbaeder/llvm-project/clang/cmake/caches/3-stage-base.cmake:2 (include)
  /home/tbaeder/llvm-project/clang/cmake/caches/3-stage.cmake:16 (include)

but also sets CMAKE_HOST_SYSTEM_PROCESSOR, which is just uname -m on Linux it seems.

The other variables printed from your snippet do not seem very useful in this case:

-- CLANG_BOOTSTRAP_TARGETS=clang;check-all;check-llvm;check-clang;test-suite;stage3;stage3-clang;stage3-check-all;stage3-check-llvm;stage3-check-clang;stage3-test-suite
-- CMAKE_BINARY_DIR=/home/tbaeder/llvm-project/build-stage3
-- CMAKE_COMMAND=/usr/bin/cmake
-- CMAKE_CPACK_COMMAND=/usr/bin/cpack
-- CMAKE_CTEST_COMMAND=/usr/bin/ctest
-- CMAKE_CURRENT_BINARY_DIR=/home/tbaeder/llvm-project/build-stage3
-- CMAKE_CURRENT_LIST_DIR=/home/tbaeder/llvm-project/clang/cmake/caches
-- CMAKE_CURRENT_LIST_FILE=/home/tbaeder/llvm-project/clang/cmake/caches/3-stage-base.cmake
-- CMAKE_CURRENT_SOURCE_DIR=/home/tbaeder/llvm-project/llvm
-- CMAKE_FILES_DIRECTORY=/CMakeFiles
-- CMAKE_HOST_SYSTEM_NAME=Linux
-- CMAKE_HOST_UNIX=1
-- CMAKE_MAJOR_VERSION=3
-- CMAKE_MINOR_VERSION=17
-- CMAKE_PARENT_LIST_FILE=/home/tbaeder/llvm-project/clang/cmake/caches/3-stage.cmake
-- CMAKE_PATCH_VERSION=4
-- CMAKE_ROOT=/usr/share/cmake
-- CMAKE_SOURCE_DIR=/home/tbaeder/llvm-project/llvm
-- CMAKE_TWEAK_VERSION=0
-- CMAKE_VERSION=3.17.4
-- LLVM_TARGETS_TO_BUILD=Native
-- UNIX=1

[tbaeder]

Here's a version that shells out to uname -m to get the arch. Setting both the _LLD and _LTO variable that often looks a bit odd but I didn't want to use other variables. Can change that of course.

[beanz]

LGTM.

The *_LTO and *_LLD variables are usable independently of each other even on non-Apple platforms, so it makes sense to need to set them both. LLVM’s LTO is supported in Gold, and I believe our build system supports MSVC’s LTCG (which is their version of LTO).

clang/CMakeLists.txt

if(BOOTSTRAP_LLVM_BUILD_INSTRUMENTED)
     set(NEXT_CLANG_STAGE ${NEXT_CLANG_STAGE}-instrumented)
   endif()
   message(STATUS "Setting next clang stage to: ${NEXT_CLANG_STAGE}")
 
 
   set(STAMP_DIR ${CMAKE_CURRENT_BINARY_DIR}/${NEXT_CLANG_STAGE}-stamps/)
   set(BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/${NEXT_CLANG_STAGE}-bins/)
 
+  # We want LLD for LTO, but LLD does not support SystemZ, so disable
+  # LTO here and use the installed system linker
+  if ("${LLVM_NATIVE_ARCH}" MATCHES "SystemZ")
+    if (BOOTSTRAP_LLVM_ENABLE_LLD OR BOOTSTRAP_LLVM_ENABLE_LTO)
+      message(STATUS "Disabling LTO and LLD for stage3 builds since LLD does not support ${LLVM_NATIVE_ARCH}")
+    endif()
+    set(BOOTSTRAP_LLVM_ENABLE_LLD OFF)
+    set(BOOTSTRAP_LLVM_ENABLE_LTO OFF)
+  elseif(APPLE)
+    # Use LLD to have fewer requirements on system linker, unless we're on an apple
+    # platform where the system compiler is to be preferred
+    message(STATUS "Using system linker for stage3 builds on Apple")
+    set(BOOTSTRAP_LLVM_ENABLE_LLD OFF)
+  else()
+    set(BOOTSTRAP_LLVM_ENABLE_LLD ON)
+    set(BOOTSTRAP_LLVM_ENABLE_LTO ON)
+  endif()
+  message(STATUS "Stage3 builds: LLD: ${BOOTSTRAP_LLVM_ENABLE_LLD}. LTO: ${BOOTSTRAP_LLVM_ENABLE_LTO}")
+
   if(BOOTSTRAP_LLVM_ENABLE_LLD)
+    # adding lld to clang-bootstrap-deps without having it enabled in
+    # LLVM_ENABLE_PROJECTS just generates a cryptic error message.
+    if (NOT "lld" IN_LIST LLVM_ENABLE_PROJECTS)
+      message(FATAL_ERROR "LLD is enabled in the boostrap build, but lld is not in LLVM_ENABLE_PROJECTS")
+    endif()
     add_dependencies(clang-bootstrap-deps lld)
   endif()
 
   # If the next stage is LTO we need to depend on LTO and possibly lld or LLVMgold
   if(BOOTSTRAP_LLVM_ENABLE_LTO OR LLVM_ENABLE_LTO AND NOT LLVM_BUILD_INSTRUMENTED)
     if(APPLE)
       add_dependencies(clang-bootstrap-deps LTO)
       # on Darwin we need to set DARWIN_LTO_LIBRARY so that -flto will work

clang/cmake/caches/3-stage-base.cmake

 set(CMAKE_BUILD_TYPE RELEASE CACHE STRING "")
 set(CLANG_ENABLE_BOOTSTRAP ON CACHE BOOL "")
 set(LLVM_BUILD_EXTERNAL_COMPILER_RT ON CACHE BOOL "")
 set(BOOTSTRAP_LLVM_ENABLE_LTO ON CACHE BOOL "")
 
-# Use LLD do have less requirements on system linker, unless we're on an apple
-# platform where the system compiler is to be prefered.
-if(APPLE)
-    set(BOOTSTRAP_LLVM_ENABLE_LLD OFF CACHE BOOL "")
-else()
-    set(BOOTSTRAP_LLVM_ENABLE_LLD ON CACHE BOOL "")
-endif()
-
-
 set(CLANG_BOOTSTRAP_TARGETS
   clang
   check-all
   check-llvm
   check-clang
   test-suite CACHE STRING "")
 
 set(CLANG_BOOTSTRAP_CMAKE_ARGS
   -C ${CMAKE_CURRENT_LIST_DIR}/3-stage-base.cmake
   CACHE STRING "")