Page MenuHomePhabricator

[CMake] Fix `is_llvm_target_library` and support out-of-order components
ClosedPublic

Authored by beanz on Jan 18 2017, 12:32 PM.

Details

Summary

This patch is required by D28855, and enables us to rely on CMake's ability to handle out of order target dependencies.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz created this revision.Jan 18 2017, 12:32 PM
bryant edited edge metadata.Jan 19 2017, 6:24 PM

Just a quick reminder.

cmake/modules/LLVM-Config.cmake
210 ↗(On Diff #84877)
list(APPEND expanded_components "$<LINK_ONLY:LLVM${c}>")
bryant added inline comments.Jan 19 2017, 7:31 PM
cmake/modules/LLVM-Config.cmake
33 ↗(On Diff #84877)

(See the inline comment after this one for justication.)

@@ -32,6 +32,28 @@ function(is_llvm_target_library library return_var)
   endforeach()
 endfunction(is_llvm_target_library)

+function(is_omitted_target_lib library return_var)
+  set(${return_var} OFF PARENT_SCOPE)
+  string(TOUPPER "${library}" capitalized_lib)
+  set(omitted_targets ${LLVM_ALL_TARGETS})
+  list(REMOVE_ITEM omitted_targets ${LLVM_TARGETS_TO_BUILD})
+  string(TOUPPER "${omitted_targets}" targets)
+  foreach(t ${targets})
+    if( capitalized_lib STREQUAL t OR
+        capitalized_lib STREQUAL "${t}" OR
+        capitalized_lib STREQUAL "${t}DESC" OR
+        capitalized_lib STREQUAL "${t}CODEGEN" OR
+        capitalized_lib STREQUAL "${t}ASMPARSER" OR
+        capitalized_lib STREQUAL "${t}ASMPRINTER" OR
+        capitalized_lib STREQUAL "${t}DISASSEMBLER" OR
+        capitalized_lib STREQUAL "${t}INFO" OR
+        capitalized_lib STREQUAL "${t}UTILS" OR
+        capitalized_lib STREQUAL "${t}INSTPRINTER" )
+      set(${return_var} ON PARENT_SCOPE)
+      break()
+    endif()
+  endforeach()
+endfunction(is_omitted_target_lib)

 macro(llvm_config executable)
   cmake_parse_arguments(ARG "USE_SHARED" "" "" ${ARGN})
211 ↗(On Diff #84877)

I might be reading this wrong, but the logic here seems to be:

  • Check if $c is a target lib (i.e., LLVMAArch64Utils) regardless of whether its corresponding target will be built;
    • If it is, then ignore it.
    • Otherwise, assume that it's a yet-to-be-scanned library.

This seems a bit off, since I recall you mentioning that "adding target libraries that are configured out is a bad idea." I think what we need is:

  • Check if $c is a target lib that will not be built.
    • If it is, ignore it.
    • Otherwise, it's either a yet-to-be-scanned {normal,target} lib.
@@ -201,12 +223,12 @@ function(llvm_map_components_to_libnames out_libs)
       list(FIND capitalized_libs LLVM${capitalized} lib_idx)
       if( lib_idx LESS 0 )
         # The component is unknown. Maybe is an omitted target?
-        is_llvm_target_library(${c} iltl_result)
-        if( NOT iltl_result )
-          # If it is not an omitted target we should assume it is a component
-          # that hasn't yet been processed by CMake. Missing components will
-          # cause errors later in the configuration, so we can safely assume
-          # that this is valid here.
+        is_omitted_target_lib(${c} iltl_result)    # see above inline comment
+        if(iltl_result)
+          message(FATAL_ERROR "Depended on ${c}, whose target isn't builts.")
+        else()
+          # either target lib or a normal lib that will be built but has yet to
+          # be scanned.
           list(APPEND expanded_components "$<LINK_ONLY:LLVM${c}>")
         endif()
       else( lib_idx LESS 0 )

This seems to generate the right build.ninja modulo reordered dependencies.

beanz updated this revision to Diff 85607.Jan 24 2017, 10:27 AM

Updating patch with a few new things.

  • Added an enforcement mechanism to ensure that target libraries aren't specified before all target libraries are created. We have this by convention today, the enforcment just creates an error.
  • Also took inspiration from bryant in D28855, to further clean up and improve the logic around is_llvm_target_library.
bryant added inline comments.Jan 24 2017, 11:15 AM
cmake/modules/LLVM-Config.cmake
12 ↗(On Diff #85607)

Could there be some documentation for this function? Something like:

# Checks if ${library} is a member component of some target in a list of
# targets. This target list is one of:
#     ALL_TARGETS (the default, if no target list specifier is given);
#     INCLUDED_TARGETS;
#     OMITTED_TARGETS.

Feel free to word-smith that.

121 ↗(On Diff #85607)

Just a small nit, not mission-critical: Could LLVM_TARGET_REGISTRATION_COMPLETE (both the local var and global prop) be shortened to, say, LLVM_TARGETS_REGISTERED, LLVM_SCANNED_TARGETS, LLVM_GOT_ALL_TARGETS? It's kind of a finger-ful to type.

128 ↗(On Diff #85607)

If we know that all targets have been scanned, can we bypass this check entirely?

if(NOT LLVM_TARGET_REGIS)  # sorry, that name is just too long
  foreach(c ${link_components})
    is_llvm_target_specifier(..)
      if(iltl_result)
        message(FATAL_ERROR ...)
      endif()
    endforeach()
endif(NOT LLVM_TARGET_REG)
242 ↗(On Diff #85607)

If we arrive here, it's precisely because one of the dependencies is a sub-component of an omitted target, right? Can you make the error message more specific to reflect this?

248 ↗(On Diff #85607)

list(APPEND expanded_components "$<LINK_ONLY:LLVM${c}>") ? Then we won't have to re-process the resulting list in the caller.

lib/Target/CMakeLists.txt
25 ↗(On Diff #85607)

CC from IRC:

9:12:17 < zq> beanz: beanz[m] crazy idea: if we're gonna require that target-depending components be scanned _after_ lib/Targets
19:12:29 < zq> then why even bother with those synonyms?
19:12:52 < zq> beanz: beanz[m] like ALLTARGETS and so forth
19:13:21 < zq> beanz: beanz[m] we could set those as special vars in lib/TArget/CMakeLists.txt right around where LLVM_TARGET_REGISTRATION_COMPLETE is set
beanz added inline comments.Jan 25 2017, 4:07 PM
cmake/modules/LLVM-Config.cmake
12 ↗(On Diff #85607)

Will do.

121 ↗(On Diff #85607)

LLVM_HAZ_ALL_TARGETS has a nice ring to it :-).

You're right that is a finger-full. I'll shorten it.

128 ↗(On Diff #85607)

Yes, that is a better expression of the logic.

242 ↗(On Diff #85607)

Will do.

248 ↗(On Diff #85607)

No, we can't do that here. There are places where this function is called where that generator expression is inappropriate (see: tools/llvm-shlib/CMakeLists.txt).

lib/Target/CMakeLists.txt
25 ↗(On Diff #85607)

There is value to the synonyms, but I have some ideas for how we can improve them. I think that shouldn't be part of this patch though.

beanz updated this revision to Diff 85957.Jan 26 2017, 1:16 PM

Updates based on review by bryant

bryant added inline comments.Jan 27 2017, 12:40 PM
cmake/modules/LLVM-Config.cmake
192 ↗(On Diff #85957)

Were you also planning to add the missing target sub-components in this patch? I think LLVM${c}Utils is missing from here.

beanz updated this revision to Diff 87486.Feb 7 2017, 11:20 AM

Adding Info and Utils handling.

This function needs a massive cleanup, but that should be a separate patch.

bryant accepted this revision.Feb 8 2017, 12:38 PM

LGTM

This revision is now accepted and ready to land.Feb 8 2017, 12:38 PM
This revision was automatically updated to reflect the committed changes.
pelikan added a subscriber: pelikan.Feb 8 2017, 5:57 PM

FYI this probably broke a clang-9999 build on Gentoo.

  • Clang version: 5.0.0
  • Performing Test CXX_SUPPORTS_NO_NESTED_ANON_TYPES_FLAG
  • Performing Test CXX_SUPPORTS_NO_NESTED_ANON_TYPES_FLAG - Failed

CMake Error at /usr/lib64/cmake/llvm/LLVM-Config.cmake:140 (message):

Specified target library before target registration is complete.

Call Stack (most recent call first):

/usr/lib64/cmake/llvm/LLVM-Config.cmake:100 (llvm_map_components_to_libnames)
/usr/lib64/cmake/llvm/LLVM-Config.cmake:93 (explicit_llvm_config)
/usr/lib64/cmake/llvm/AddLLVM.cmake:713 (llvm_config)
cmake/modules/AddClang.cmake:125 (add_llvm_executable)
examples/clang-interpreter/CMakeLists.txt:11 (add_clang_executable)

The CMake invocation was as follows:

Working in BUILD_DIR: "/var/tmp/portage/sys-devel/clang-9999/work/clang-9999-abi_x86_64.amd64"

cmake -C /var/tmp/portage/sys-devel/clang-9999/work/clang-9999-abi_x86_64.amd64/gentoo_common_config.cmake -G Ninja -DCMAKE_INSTALL_PREFIX=/usr -DLLVM_CONFIG=/usr/bin/x86_64-pc-linux-gnu-llvm-config -DCLANG_RESOURCE_DIR=../lib/clang/5.0.0 -DBUILD_SHARED_LIBS=ON -DLLVM_TARGETS_TO_BUILD=AMDGPU;AArch64;ARM;BPF;Hexagon;Lanai;Mips;MSP430;NVPTX;PowerPC;RISCV;Sparc;SystemZ;X86;XCore -DLLVM_BUILD_TESTS=no -DLLVM_ENABLE_EH=ON -DLLVM_ENABLE_RTTI=ON -DCMAKE_DISABLE_FIND_PACKAGE_LibXml2=yes -DCLANG_DEFAULT_OPENMP_RUNTIME=libomp -DCLANG_DEFAULT_CXX_STDLIB=libc++ -DCLANG_DEFAULT_RTLIB=compiler-rt -DCLANG_ENABLE_ARCMT=yes -DCLANG_ENABLE_STATIC_ANALYZER=yes -DLLVM_BUILD_DOCS=no -DLLVM_ENABLE_SPHINX=no -DLLVM_ENABLE_DOXYGEN=OFF -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_USER_MAKE_RULES_OVERRIDE=/var/tmp/portage/sys-devel/clang-9999/work/clang-9999-abi_x86_64.amd64/gentoo_rules.cmake -DCMAKE_TOOLCHAIN_FILE=/var/tmp/portage/sys-devel/clang-9999/work/clang-9999-abi_x86_64.amd64/gentoo_toolchain.cmake /var/tmp/portage/sys-devel/clang-9999/work/clang-9999