Page MenuHomePhabricator

[cmake] With utils disabled, don't build tblgen in cross mode
ClosedPublic

Authored by loladiro on Jul 1 2019, 1:53 PM.

Details

Summary

In cross mode, we build a separate NATIVE tblgen that runs on the
host and is used during the build. Separately, we have a flag that
disables building all executables in utils/. Of course generally,
this doesn't turn off tblgen, since we need that during the build.
In cross mode, however, that tblegen is useless since we never
actually use it. Furthermore, it can be actively problematic if the
cross toolchain doesn't like building executables for whatever reason.
And even if building executables works fine, we can at least save
compile time by omitting it from the target build. There's two changes
needed to make this happen:

  • Stop creating a dependency from the native tool to the target tool. No such dependency is required for a correct build, so I'm not entirely sure why it was there in the first place.
  • If utils were disabled on the CMake command line and we're in cross mode, respect that by excluding it from the install target (using EXCLUDE_FROM_ALL).

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro created this revision.Jul 1 2019, 1:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2019, 1:53 PM

Thanks for working on this!

llvm/cmake/modules/TableGen.cmake
125 ↗(On Diff #207404)

Thank you for fixing this.

142 ↗(On Diff #207404)

I'm not a fan of this, especially since add_tablegen is a macro, so setting this variable will leak out of it.

Unfortunately, it doesn't seem like there's any other way to pass this information to add_llvm_executable. Ideally, you'd add a new argument to that method and use it here. Failing that, you should save the old value of EXCLUDE_FROM_ALL and restore it after your add_llvm_executable call, similar to what's being done for LLVM_LINK_COMPONENTS.

loladiro updated this revision to Diff 207424.Jul 1 2019, 3:45 PM

Use set_target_properties to set EXCLUDE_FROM_ALL on the target rather than using
the global setting.

smeenai accepted this revision.Jul 1 2019, 3:54 PM

LGTM

This revision is now accepted and ready to land.Jul 1 2019, 3:54 PM
This revision was automatically updated to reflect the committed changes.
beanz added a subscriber: beanz.Aug 5 2019, 10:55 AM

This patch is actually really problematic. We have the native tablegen depend on the target tablegen so that the native one rebuilds whenever the sources for the target one rebuild. Without that dependency the native tablegen may not rebuild at all, or (if it does) it will likely rebuild at the wrong time.

This patch makes iterative builds on a cross-target incorrect.

beanz added a comment.Aug 5 2019, 11:07 AM

This patch is effectively un-done as of rL367895. I'm happy to discuss alternative approaches to the problem you're trying to solve, but we need correct builds, which means we need the dependency between the native and target tool.

Is there a way to just add the whatever the underlying dependencies are that are missing for the native tool? My concern is that I don't want to build target executables (because the toolchain for that is broken).

beanz added a comment.Aug 5 2019, 12:27 PM

You'd need to make the NATIVE target depend on all the source files and headers that the the target depends on, which is theoretically possible, although I'm not sure of a clean way to do it in CMake.

If your target toolchain doesn't support executables you can always feed in a pre-built tablegen, which will skip all of this, and skip building one, but then it is on you to ensure that your tablegen is getting rebuilt properly.

Hmm, we do have a list of all the .cpp files in the tablegen tool in ${ARGN}, and I'd be fine depending on the target libraries. Do you know of a way to emulate whatever handling add_executable is doing for .cpp files to get at the included .h files?

beanz added a comment.Aug 5 2019, 2:41 PM

I don't know of any way to emulate that because it isn't something CMake exports or even knows about. It is something the underlying build system detects from the compiler's depfiles. One thought that might work would be to generate an object library target containing the tool source files, and instead depend on the object library and all the target static archives. I think that would get the ordering right and would not require the link step to run.

Ok, I think that'd work for my use case. I'll try it out and post a revision to that extent.