Page MenuHomePhabricator

Make TableGenGlobalISel an object library
ClosedPublic

Authored by aaronpuchert on Feb 13 2020, 7:10 PM.

Details

Summary

That's how it was originally intended but that wasn't possible because
we still needed to support older CMake versions.

The problem here is that the sources in TableGenGlobalISel are meant to
be linked into both llvm-tblgen and TableGenTests (a unit test), but not
be part of LLVM proper. So they shouldn't be an ordinary LLVM component.
Because they are used in llvm-tblgen, they can't draw in the LLVM dylib
dependency, but then we'd have to do the same thing in TableGenTests to
make sure we don't link both a static Support library and another copy
through the LLVM dylib.

With an object library we're just reusing the object files and don't
have to care about dependencies at all.

Event Timeline

aaronpuchert created this revision.Feb 13 2020, 7:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2020, 7:10 PM

Ping. This is similar to the fix in rL373651 over the original commit rL373551.

beanz added a comment.Mar 27 2021, 7:48 AM

I think this bug is the other way around. LLVMTableGenGlobalISel should not be using DISABLE_LLVM_LINK_LLVM_DYLIB.

aaronpuchert added a comment.EditedMar 27 2021, 2:15 PM

I think this bug is the other way around. LLVMTableGenGlobalISel should not be using DISABLE_LLVM_LINK_LLVM_DYLIB.

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?

aaronpuchert added a subscriber: thakis.EditedMar 27 2021, 3:24 PM

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

beanz added a comment.Mar 29 2021, 6:45 PM

I think this bug is the other way around. LLVMTableGenGlobalISel should not be using DISABLE_LLVM_LINK_LLVM_DYLIB.

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.

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.

I think this bug is the other way around. LLVMTableGenGlobalISel should not be using DISABLE_LLVM_LINK_LLVM_DYLIB.

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.

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.

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.

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.

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 .

beanz added a comment.Mar 30 2021, 8:56 AM

So apparently we do import the LLVM dependency from LLVMTableGenGlobalISel .

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

Yep, and that's because of a different bug in how LLVMTableGenGlobalISel was setup.

This patch would address the issues: [...]

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.
  • maybe it would be confusing to have an LLVM component in llvm/utils instead of llvm/lib, and

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.

Yep, and that's because of a different bug in how LLVMTableGenGlobalISel was setup.

This patch would address the issues: [...]

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.

Yeah, I agree this doesn't belong in llvm/lib. They're llvm-tblgen sources.

  • maybe it would be confusing to have an LLVM component in llvm/utils instead of llvm/lib, and

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.

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.

All I wanted was a subdirectory to organize the sources specific to the GISel tablegen passes :-).

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.

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.

All I wanted was a subdirectory to organize the sources specific to the GISel tablegen passes :-).

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 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?

beanz added a comment.Mar 31 2021, 9:25 AM

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.

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
diff --git a/llvm/unittests/TableGen/CMakeLists.txt b/llvm/unittests/TableGen/CMakeLists.txt
--- a/llvm/unittests/TableGen/CMakeLists.txt
+++ b/llvm/unittests/TableGen/CMakeLists.txt
-target_link_libraries(TableGenTests PRIVATE LLVMTableGenGlobalISel LLVMTableGen)
+target_link_libraries(TableGenTests PRIVATE LLVMTableGen)

I think we can also drop LLVMTableGen since it's in LLVM_LINK_COMPONENTS already?

Go back to object library instead.

aaronpuchert retitled this revision from Use DISABLE_LLVM_LINK_LLVM_DYLIB for TableGenTests to Make TableGenGlobalISel an object library.Mar 31 2021, 11:10 AM
aaronpuchert edited the summary of this revision. (Show Details)
beanz accepted this revision.Mar 31 2021, 11:24 AM

LGTM.

This revision is now accepted and ready to land.Mar 31 2021, 11:24 AM
diff --git a/llvm/unittests/TableGen/CMakeLists.txt b/llvm/unittests/TableGen/CMakeLists.txt
--- a/llvm/unittests/TableGen/CMakeLists.txt
+++ b/llvm/unittests/TableGen/CMakeLists.txt
-target_link_libraries(TableGenTests PRIVATE LLVMTableGenGlobalISel LLVMTableGen)
+target_link_libraries(TableGenTests PRIVATE LLVMTableGen)

I think we can also drop LLVMTableGen since it's in LLVM_LINK_COMPONENTS already?

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.

This revision was landed with ongoing or failed builds.Mar 31 2021, 1:22 PM
This revision was automatically updated to reflect the committed changes.

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?

aaronpuchert reopened this revision.Mar 31 2021, 6:07 PM
This revision is now accepted and ready to land.Mar 31 2021, 6:07 PM

Let's do it the CMake 3.13 way, that seems to work even with the object library hack in add_tablegen.

@beanz, Phabricator still treats this as accepted, but I'd appreciate if you have another look at it.

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.

Reverting back to original change. I think it's the only thing that works.

This revision was automatically updated to reflect the committed changes.