Page MenuHomePhabricator

[CMake] Subsume LLVMBuild.txt. Disabled by default.
Needs ReviewPublic

Authored by bryant on Dec 23 2016, 6:42 PM.

Details

Summary

With this patch, llvmbuild no longer needs to be invoked during cmake runs.

Notes:

  • Try this by passing WITHOUT_LLVMBUILD=ON, e.g.,

    `bash $ mkdir build; cd build $ cmake -G Ninja -D WITHOUT_LLVMBUILD=ON ${other_cmake_flags} .. $ ninja as usual ``
  • LLVM components are expected to be libraries whose names begin with "LLVM," case sensitive.
  • Component dependencies are now passed into LLVM_-/LINK_COMPONENTS. Either:

    `cmake set(LLVM_LINK_COMPONENTS LLVMLibraryName Core Support Demangle ...) add_llvm_library(LLVMLibraryName src0.cpp src1.cpp ...) `

    or

    `cmake add_llvm_library(LLVMLibraryName src0.cpp src1.cpp ... LINK_COMPONENTS LLVMLibraryName Core Support Demangle ... ) `

    Arguments to *LINK_COMPONENTS are now case sensitive because we no longer have the benefit of llvmbuild's pre-computed dependencies.
  • LibraryDependencies.inc, required by llvm-config, is now built by LLVMBuild.cmake after all component dependencies are collected and checked

Diff Detail

Repository
rL LLVM

Event Timeline

bryant updated this revision to Diff 82430.Dec 23 2016, 6:42 PM
bryant retitled this revision from to [CMake] Subsume LLVMBuild.txt. Disabled by default..
bryant updated this object.
bryant added a reviewer: beanz.
bryant set the repository for this revision to rL LLVM.
bryant added a subscriber: llvm-commits.
bryant updated this revision to Diff 82678.Dec 29 2016, 10:18 AM
bryant edited edge metadata.
  • Use the correct variable: LLVM_TARGETS_TO_BUILD, not LLVM_ALL_TARGETS.
  • Removed accidental llvmbuild diff.
bryant updated this revision to Diff 82683.Dec 29 2016, 11:32 AM
bryant edited edge metadata.

Fixed BPFDisassembler's LLVM_LINK_COMPONENTS.

beanz edited edge metadata.Jan 9 2017, 12:11 PM

Hey @bryant,

Thank you so much for taking a stab at this. I'm sorry I've been OOO and unable to review it. I'm going to try and take this patch for a spin this week. I think we may be able to simplify and reduce it a bit, and I'll try and get a full review to you later this week.

If you don't hear anything from me by Monday please harass me either here or on IRC.

Thanks!
-Chris

chapuni added a subscriber: chapuni.

I don't understand why you suggest WITHOUT_LLVMBUILD. We can just prune llvmbuild.

You may do the job step-by-step,

  1. Let llvm-config free from llvmbuild
  2. Migrate deps from LLVMBuild.txt(s)
  3. Prune llvmbuild

I also think LINK_COMPONENTS would be redundant since it comes from compatibility for autoconf.
We may take LLVMSupport instead of Support, for example.

That said, I am not a fan of direct use of target_link_libraries. It is not controllable.
I suggest to use LINK_LIBS in llvm_add_library().

I don't understand why you suggest WITHOUT_LLVMBUILD. We can just prune
llvmbuild.

It's a flag to toggle between the old behavior (that uses LLVMBuild) and new
(that uses functionality added by this patch).

You may do the job step-by-step,

Let llvm-config free from llvmbuild
Migrate deps from LLVMBuild.txt(s)
Prune llvmbuild

The first two is exactly what this patch does with WITHOUT_LLVMBUILD=ON. The
third -- deleting llvmbuild-related files -- can be done in a separate patch
once it's been verified that this patch adequately replicates the existing
behavior in pure CMake.

I also think LINK_COMPONENTS would be redundant since it comes from
compatibility for autoconf. We may take LLVMSupport instead of Support, for
example.

When telling CMake to build a library LLVMxyz, there needs to be a distinction
between 1) stuff that needs to be linked into with xyz _that are themselves
LLVM*_, and 2) other stuff that xyz needs but aren't themselves LLVM*.

For instance, LLVMCodeGen requires LLVM{Analysis,BitReader,...} (first category;
call them LLVM-internal stuff) and ${PTHREAD_LIB} (second category).

This distinction is needed in order to generate LibraryDependencies.inc, which
records dependencies of the first but not the second kind.

That said, I am not a fan of direct use of target_link_libraries. It is not
controllable. I suggest to use LINK_LIBS in llvm_add_library().

You're right: It's possible to entirely deprecate
LINK_COMPONENTS/LLVM_LINK_COMPONENTS in favor of LINK_LIBS. In that case, we'd
need to look out for LINK_LIBs that match the regex ^LLVM.+$ and handle them
slightly differently for LibraryDependencies.inc. But such special handling
machinery already exists in the form of LINK_COMPONENTS.

In other words, this patch expects all LLVM-internal dependencies to be
specified in LINK_COMPONENTS and all other deps under LINK_LIBS.

Please let me know if that made sense to you.

beanz added a comment.Jan 17 2017, 1:55 PM

@bryant and I spoke over IRC in a PM, for posterity I'll recap here.

@chapuni I also thought using LINK_LIBS would make sense, but that is actually more complicated because we have special "magic" values that work for components like the "Native" target that we still need to support. If we want to get rid of the notion of special-purpose components I think we should consider that separately.

I've suggested that this patch be split into two patches.

One that adds missing dependencies by adding LINK_COMPONENTS as named arguments to the add_llvm_* calls, which can be done always with no conditionals, and should be NFC. This patch will result in filling in the missing gaps where our CMake currently does't express the full dependency tree.

Then a second patch which adds support for generating the LinkDependencies.inc file from CMake's dependency information.

There may be a few small CMake issues that pop up and need to be addressed between the first and second patch, but we can work through that as they come up.