We need this because the test links with LLVMTableGenGlobalISel which
has the same option. Without this the test would link with the LLVM
dylib and a static LLVMSupport, which results in duplicate flags.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 46467 Build 48945: arc lint + arc unit
Event Timeline
Comment Actions
I think this bug is the other way around. LLVMTableGenGlobalISel should not be using DISABLE_LLVM_LINK_LLVM_DYLIB.
Comment Actions
The commit message of rL373651 says
Fixed the -DLLVM_LINK_LLVM_DYLIB=ON using DISABLE_LLVM_LINK_LLVM_DYLIB when creating the library. Apparently it automatically links to libLLVM.dylib and we don't want that from tablegen.
Originally it didn't have this option and was an object library in @dsanders' original change D68288. But at that time we couldn't use object libraries because older CMake versions were still supported. (See the discussion on the change.)
By now I think we require CMake 3.13.4 and could go back to the object library?
Comment Actions
@thakis, now that we require CMake 3.13.4, would you be fine with going back to an object library?
Otherwise LLVMTableGenGlobalISel has to go with DISABLE_LLVM_LINK_LLVM_DYLIB, and then also this test.
Comment Actions
This makes no sense. The tablegen tool build disables linking the dylib, which should be sufficient. We should never need to force it on a static archive. I’m 99% sure, that rL373651 is just wrong here, and that the solution to all of this is to just remove DISABLE_LLVM_LINK_LLVM_DYLIB from the library.
Comment Actions
The buildbot logs are long gone now but the only reason I added it was because I had twenty-odd bots failing on the object library and this was the only way I could get both local builds and bot builds to work.
I'd hazard a guess it originated from from this in llvm/unittests/CMakeLists.txt
# The target unittests may test APIs that aren't exported in libLLVM.so, so
# we need to always link against the static libraries.
function(add_llvm_target_unittest test_dir_name)
add_llvm_unittest(${test_dir_name} DISABLE_LLVM_LINK_LLVM_DYLIB ${ARGN})
endfunction()or at least has a similar issue but the only way to be sure is to remove it again and see what the bots complained about again.
Comment Actions
When I remove it, CMake complains about cyclic dependencies. Here is the gist (I reordered the parts a bit):
CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
"llvm-tblgen" of type EXECUTABLE
depends on "LLVMTableGenGlobalISel" (weak)
depends on "LLVM" (weak)
"LLVMTableGenGlobalISel" of type STATIC_LIBRARY
depends on "LLVM" (weak)
"LLVM" of type SHARED_LIBRARY
depends on "LLVMCore" (weak)
[... and many more ...]
"LLVMCore" of type STATIC_LIBRARY
depends on "LLVMRemarks" (weak)
depends on "intrinsics_gen" (strong)
"intrinsics_gen" of type UTILITY
depends on "llvm-tblgen" (strong)
At least one of these targets is not a STATIC_LIBRARY. Cyclic dependencies are allowed only among static libraries.So apparently we do import the LLVM dependency from LLVMTableGenGlobalISel .
Comment Actions
Yep, and that's because of a different bug in how LLVMTableGenGlobalISel was setup.
This patch would address the issues:
diff --git a/llvm/utils/TableGen/CMakeLists.txt b/llvm/utils/TableGen/CMakeLists.txt index 9e918852b1c0..c05252a756ce 100644 --- a/llvm/utils/TableGen/CMakeLists.txt +++ b/llvm/utils/TableGen/CMakeLists.txt @@ -1,6 +1,6 @@ add_subdirectory(GlobalISel) -set(LLVM_LINK_COMPONENTS Support) +set(LLVM_LINK_COMPONENTS Support TableGenGlobalISel) add_tablegen(llvm-tblgen LLVM AsmMatcherEmitter.cpp @@ -57,5 +57,4 @@ add_tablegen(llvm-tblgen LLVM WebAssemblyDisassemblerEmitter.cpp CTagsEmitter.cpp ) -target_link_libraries(llvm-tblgen PRIVATE LLVMTableGenGlobalISel) set_target_properties(llvm-tblgen PROPERTIES FOLDER "Tablegenning") diff --git a/llvm/utils/TableGen/GlobalISel/CMakeLists.txt b/llvm/utils/TableGen/GlobalISel/CMakeLists.txt index c23ef6742f32..5c97e66c8a33 100644 --- a/llvm/utils/TableGen/GlobalISel/CMakeLists.txt +++ b/llvm/utils/TableGen/GlobalISel/CMakeLists.txt @@ -3,7 +3,7 @@ set(LLVM_LINK_COMPONENTS TableGen ) -add_llvm_library(LLVMTableGenGlobalISel STATIC DISABLE_LLVM_LINK_LLVM_DYLIB +add_llvm_component_library(LLVMTableGenGlobalISel STATIC CodeExpander.cpp GIMatchDag.cpp GIMatchDagEdge.cpp
Comment Actions
Ok, that allows me to remove the DISABLE_LLVM_LINK_LLVM_DYLIB, but
- maybe it would be confusing to have an LLVM component in llvm/utils instead of llvm/lib, and
- maybe this shouldn't be part of LLVM because it's only needed for llvm-tblgen? In other words maybe this isn't actually a component of LLVM proper but just a tool for building LLVM. If we make it a component library it will be part of libLLVM.so.
Comment Actions
Yea, that is confusing. It is also confusing that LLVMTableGenGlobalISel was put into a static archive at all instead of just making it part of the tablegen executable like all the other tablegen backends and utils.
- maybe this shouldn't be part of LLVM because it's only needed for llvm-tblgen? In other words maybe this isn't actually a component of LLVM proper but just a tool for building LLVM. If we make it a component library it will be part of libLLVM.so.
LLVMTableGenGlobalISel is trying to have its cake and eat it to. You can't make a library that you expect to behave like an LLVM component for linkages, but then not make it a component. LLVMTableGen is an LLVM component, even though it is really only used for building LLVM too, so there is prior art here.
The alternative to making LLVMTableGenGlobalISel would be to hand roll the proper linkage behavior so that it doesn't pull in libLLVM.
Comment Actions
Yeah, I agree this doesn't belong in llvm/lib. They're llvm-tblgen sources.
All I wanted was a subdirectory to organize the sources specific to the GISel tablegen passes :-). I'd have been happy with GlobalISel/Foo.cpp in the llvm/utils/TableGen/CMakeLists.txt but CMake objects to that. I then went with the object library so that CMake would build the objects that I could then link as part of llvm-tblgen instead of being a proper library but the bots running older CMake's objected to that.
If there's a simple way to have sources for llvm-tblgen in a subdirectory of llvm/utils/TableGen then I'd be happy to switch to it.
Comment Actions
Not only that, you also want to use them in TableGenTests, and this I think is why can't just have them in llvm-tblgen.
I'd have been happy with GlobalISel/Foo.cpp in the llvm/utils/TableGen/CMakeLists.txt but CMake objects to that.
I'm sure CMake shouldn't complain about that, the unit test dependency should be the issue.
I then went with the object library so that CMake would build the objects that I could then link as part of llvm-tblgen instead of being a proper library but the bots running older CMake's objected to that.
That I think should not be a problem anymore.
To summarize: these are sources that are not part of LLVM proper but of a tool, but you want to unit test them, so you need them as a separate target that you can use for both. It appears to be unconventional to have such code unit tested. But I don't want to ask you to rewrite the tests as lit tests. (I have no idea whether that's feasible even.)
In my view the best solution would be to go with the original object library, which should be fine by now because we have cmake_minimum_required(VERSION 3.13.4) in llvm/CMakeLists.txt, so everybody has to use a CMake version that supports them. We're not talking about an awful number of sources, and for those not building unit tests it behaves just as if the sources were part of llvm-tblgen.
Comment Actions
On the other hand, if llvm-tblgen is not meant to link with the LLVM dylib, maybe the unit test for TableGen should also be standalone? Which would be an argument for the change as it is.
I think it wouldn't awfully hurt: the much bigger target unit tests are also not linking with the dylib, while this is just linking two source files with two of most basic components.
Comment Actions
I'd completely forgotten about the CodeExpander tests, sorry about that. Yes, you're right those do require more than just a subdirectory.
I'd have been happy with GlobalISel/Foo.cpp in the llvm/utils/TableGen/CMakeLists.txt but CMake objects to that.
I'm sure CMake shouldn't complain about that, the unit test dependency should be the issue.
FWIW, it might have been LLVM specific. What I see in the code for that at the moment would have let it through though
I then went with the object library so that CMake would build the objects that I could then link as part of llvm-tblgen instead of being a proper library but the bots running older CMake's objected to that.
That I think should not be a problem anymore.
To summarize: these are sources that are not part of LLVM proper but of a tool, but you want to unit test them, so you need them as a separate target that you can use for both. It appears to be unconventional to have such code unit tested. But I don't want to ask you to rewrite the tests as lit tests. (I have no idea whether that's feasible even.)
It's possible to convert to lit tests but being able to unit test directly saves on the extra boilerplate code. Options like -gicombiner-stop-after-parse are there for testing but they're really just early exits on what it was normally doing. The CodeExpander tests would need boilerplate to wrap it in a command line interface.
In my view the best solution would be to go with the original object library, which should be fine by now because we have cmake_minimum_required(VERSION 3.13.4) in llvm/CMakeLists.txt, so everybody has to use a CMake version that supports them. We're not talking about an awful number of sources, and for those not building unit tests it behaves just as if the sources were part of llvm-tblgen.
That sounds good to me. Would you like me to change it back to that?
Comment Actions
I'm not sure what CMake issues you encountered. This does concern me a little bit because we've used object libraries in a lot of places in the past without issue even before the CMake version bump. CMake has supported object libraries for years, the only recent change to handling them was allowing them to be specified in target_link_libraries, which isn't super important here.
In my view the best solution would be to go with the original object library, which should be fine by now because we have cmake_minimum_required(VERSION 3.13.4) in llvm/CMakeLists.txt, so everybody has to use a CMake version that supports them. We're not talking about an awful number of sources, and for those not building unit tests it behaves just as if the sources were part of llvm-tblgen.
That sounds good to me. Would you like me to change it back to that?
That sounds reasonable. Here's a patch:
diff --git a/llvm/unittests/TableGen/CMakeLists.txt b/llvm/unittests/TableGen/CMakeLists.txt index 178018ba8967..c678f2b639d9 100644 --- a/llvm/unittests/TableGen/CMakeLists.txt +++ b/llvm/unittests/TableGen/CMakeLists.txt @@ -12,6 +12,7 @@ add_public_tablegen_target(AutomataTestTableGen) add_llvm_unittest(TableGenTests CodeExpanderTest.cpp AutomataTest.cpp + $<TARGET_OBJECTS:obj.LLVMTableGenGlobalISel> ) include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../utils/TableGen) -target_link_libraries(TableGenTests PRIVATE LLVMTableGenGlobalISel LLVMTableGen) +target_link_libraries(TableGenTests PRIVATE LLVMTableGen) diff --git a/llvm/utils/TableGen/CMakeLists.txt b/llvm/utils/TableGen/CMakeLists.txt index 9e918852b1c0..8b01a40f3ad5 100644 --- a/llvm/utils/TableGen/CMakeLists.txt +++ b/llvm/utils/TableGen/CMakeLists.txt @@ -56,6 +56,6 @@ add_tablegen(llvm-tblgen LLVM X86RecognizableInstr.cpp WebAssemblyDisassemblerEmitter.cpp CTagsEmitter.cpp + $<TARGET_OBJECTS:obj.LLVMTableGenGlobalISel> ) -target_link_libraries(llvm-tblgen PRIVATE LLVMTableGenGlobalISel) set_target_properties(llvm-tblgen PROPERTIES FOLDER "Tablegenning") diff --git a/llvm/utils/TableGen/GlobalISel/CMakeLists.txt b/llvm/utils/TableGen/GlobalISel/CMakeLists.txt index c23ef6742f32..1cd741cd6be1 100644 --- a/llvm/utils/TableGen/GlobalISel/CMakeLists.txt +++ b/llvm/utils/TableGen/GlobalISel/CMakeLists.txt @@ -3,7 +3,7 @@ set(LLVM_LINK_COMPONENTS TableGen ) -add_llvm_library(LLVMTableGenGlobalISel STATIC DISABLE_LLVM_LINK_LLVM_DYLIB +add_llvm_library(LLVMTableGenGlobalISel OBJECT CodeExpander.cpp GIMatchDag.cpp GIMatchDagEdge.cpp
Comment Actions
Apparently this doesn't work with LLVM_LINK_LLVM_DYLIB=ON, I get errors like undefined symbol: llvm::PrintWarning(llvm::ArrayRef<llvm::SMLoc>, llvm::Twine const&). These are from TableGen, and indeed libLLVMTableGen.a is not linked into libLLVM.so (llvm/tools/llvm-shlib/CMakeLists.txt):
# Exclude libLLVMTableGen for the following reasons: # - it is only used by internal *-tblgen utilities; # - it pollutes the global options space. list(REMOVE_ITEM LIB_NAMES "LLVMTableGen")
and TableGenTests is only linked with gtest and the latter. This seems like we shouldn't replace LLVMTableGen with LLVM if the latter doesn't provide the former. But that's for another change, I'll go with your proposed patch that keeps the explicit link to TableGenfor now.
Comment Actions
It seems that everything is fine with ninja, but with make we're running into this fun hack:
macro(add_tablegen target project)
# ...
# CMake doesn't let compilation units depend on their dependent libraries on some generators.
if(NOT CMAKE_GENERATOR STREQUAL "Ninja" AND NOT XCODE)
# FIXME: It leaks to user, callee of add_tablegen.
set(LLVM_ENABLE_OBJLIB ON)
endif()The source list of llvm-tblgen is then as expected $<TARGET_OBJECTS:obj.llvm-tblgen>, while the source list of obj.llvm-tblgen is AsmMatcherEmitter.cpp;...;CTagsEmitter.cpp;$<TARGET_OBJECTS:obj.LLVMTableGenGlobalISel>;..., and the objects from obj.LLVMTableGenGlobalISel are missing from the linker command. Apparently it's not possible to nest object libraries this way?
@beanz, I'm currently playing around with target_link_libraries for the object library since that seems to be supported now, but perhaps you already know why this won't work?
Comment Actions
Let's do it the CMake 3.13 way, that seems to work even with the object library hack in add_tablegen.
Comment Actions
@beanz, Phabricator still treats this as accepted, but I'd appreciate if you have another look at it.
Comment Actions
Hmm, this doesn't actually work. The problem is that we're using target_link_libraries with LLVMTableGen, which we have to, because it's not in the big shared library. But then we also get the dependencies of that (LLVMSupport and LLVMDemangle), which are part of the shared library. In my experiments there wasn't a problem, but that might just be a coincidence.
So I think we have to go back to the original change.