Page MenuHomePhabricator

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

Authored by tbaeder on Oct 22 2020, 3:03 AM.

Details

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!

Diff Detail

Event Timeline

tbaeder created this revision.Oct 22 2020, 3:03 AM
tbaeder requested review of this revision.Oct 22 2020, 3:03 AM
clang/CMakeLists.txt
651

Should you also disable LTO here?

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

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

This revision is now accepted and ready to land.Mar 9 2021, 12:17 AM
tbaeder updated this revision to Diff 329309.Mar 9 2021, 5:52 AM

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

clang/CMakeLists.txt
643

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 updated this revision to Diff 330954.Mar 16 2021, 5:36 AM

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.

tbaeder marked 2 inline comments as done.Mar 16 2021, 5:36 AM
beanz added a comment.Mar 16 2021, 8:19 AM

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.

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 added a comment.Mar 16 2021, 9:08 AM

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?

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.

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 updated this revision to Diff 331246.Mar 17 2021, 5:42 AM

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 accepted this revision.Mar 17 2021, 6:25 AM

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).

This revision was landed with ongoing or failed builds.Mar 17 2021, 7:16 AM
This revision was automatically updated to reflect the committed changes.