[clang] Only build clang-tblgen if it is actually needed

[rossburton]

It's possible to build clang with an existing clang-tblgen (common when cross-compiling, for instance) by setting CLANG_TABLEGEN_EXE. If this is the case then building another copy of clang-tblgen that won't be used is pointless.

In Yocto we cross-compile the LLVM projects individually to minimise the amounts of building that is needed (no need to build clang if you just want llvm) and the first step of the build is to build optimised native tablegen binaries so that they're built once and reused often. When a tablegen binary is provided there's no need for clang to setup a native build environment and build the tablegen when the codepaths will all use the provided one and not the freshly built one.

This was discovered whilst chasing down a really horrible build failure where the native binary that clang was building itself in a cross build didn't actually compile, and the realisation that it shouldn't even be building this binary in the first place.

[rossburton]

Just marked as a draft as I'm thinking there might be an case where you want to both use an existing tblgen and build a new one: in complex multi-stage cross-compilation builds of clang. I'm not sure if this is overcomplicating things, or a realistic need.

Either way, I expect extending the check to be if CLANG_TABLEGEN_EXE and not CMAKE_CROSS_COMPILING would be sufficient?

[rossburton]

And CI exploded, so I'll dig into that obviously.

[rossburton]

So my approach was wrong. My goal is to not build clang-tblgen if it's already been provided, but not explode the build if it hasn't. Obviously I'm doing something wrong with this approach and either I need to fiddle the logic in add_tablegen or the behaviour I'm after already exists if I pass the right arguments.

Currently, if I do a non-cross build and set CLANG_TABLEGEN_EXE, it builds a tablegen binary that is not used nor installed.

[DavidSpickett]

For this cmake command:

$ cmake ../llvm-project/llvm/ -DCMAKE_BUILD_TYPE=Release -DCLANG_TABLEGEN_EXE=/home/david.spickett/build-llvm-aarch64/bin/clang-tblgen -DLLVM_CCACHE_BUILD=ON -G Ninja -DLLVM_ENABLE_PROJECTS="clang;llvm" -DLLVM_INCLUDE_TESTS=ON

This patch works for me:

diff --git a/clang/CMakeLists.txt b/clang/CMakeLists.txt
index e4cb1a359620..b650b3b986f4 100644
--- a/clang/CMakeLists.txt
+++ b/clang/CMakeLists.txt
@@ -479,7 +479,9 @@ option(CLANG_ENABLE_HLSL "Include HLSL build products" Off)
 # While HLSL support is experimental this should stay hidden.
 mark_as_advanced(CLANG_ENABLE_HLSL)
 
-add_subdirectory(utils/TableGen)
+if (NOT DEFINED CLANG_TABLEGEN_EXE OR CLANG_INCLUDE_TESTS)
+  add_subdirectory(utils/TableGen)
+endif()
 
 # Export CLANG_TABLEGEN_EXE for use by flang docs.
 set(CLANG_TABLEGEN_EXE "${CLANG_TABLEGEN_EXE}" CACHE INTERNAL "")

Even though it seems to do the same thing you did, but one level up. I wonder if it's a variable scope difference.

Regardless, we'd usually do this at the point where we'd call add_subdirectory, rather than in the subdirectory itself.

Though we do not say either way the meaning of providing and existing clang-tblgen and running tests, it does currently run tests that use the newly built clang-tblgen. It's not unreasonable to think someone would want to test the clang they'd just built, and expecting them to expect that this requires some random DSL compiler, that's a bit much.

In theory someone could be relying on there being another clang-tblgen in bin, maybe for installs, but that seems like a thing we can afford to break. They were building clang-tblgen anyway, so they could stop setting CLANG_TABLEGEN_EXE, and use the newly built one instead.

Maybe they are building debug but giving a path to release but for that, we have a specific option to not apply debug settings to tablegen.

So I think the way to think about it is, we need to build a new clang-tblgen when:

• CLANG_TABLEGEN_EXE is not provided, or -
• CLANG_INCLUDE_TESTS is ON

If you want tests on and no extra copy of clang-tblgen then that's going to require adding a new testing feature for the few clang tests that require clang-tblgen. Which isn't that difficult but I don't think we have to go that far given that most people will be disabling tests altogether if they care about build time.

Making it so that clang tests using clang-tblgen use the one from CLANG_TABLEGEN_EXE is confusing and I don't think the results would be meaningful. The in-tree tests are written expecting the newest tablegen, so it's likely the behaviour will get out of sync. So definitely don't do that.

[DavidSpickett]

I should note that llvm also has this "problem", but llvm being the first step of the chain likely you want to build llvm-tblgen anyway.

I did try applying the same kind of change there but due to the whole min-tblgen thing, it looked more complex.

There's a reasonable argument for llvm remaining "special" in this way but not anything downstream of it.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Ross Burton (rossburton)

Changes

It's possible to build clang with an existing clang-tblgen (common when cross-compiling, for instance) by setting CLANG_TABLEGEN_EXE. If this is the case there's no need to build it, as it won't be used.

---

Full diff: https://github.com/llvm/llvm-project/pull/161952.diff

1 Files Affected:

• (modified) clang/CMakeLists.txt (+3-1)

diff --git a/clang/CMakeLists.txt b/clang/CMakeLists.txt
index e4cb1a359620d..b650b3b986f4c 100644
--- a/clang/CMakeLists.txt
+++ b/clang/CMakeLists.txt
@@ -479,7 +479,9 @@ option(CLANG_ENABLE_HLSL "Include HLSL build products" Off)
 # While HLSL support is experimental this should stay hidden.
 mark_as_advanced(CLANG_ENABLE_HLSL)
 
-add_subdirectory(utils/TableGen)
+if (NOT DEFINED CLANG_TABLEGEN_EXE OR CLANG_INCLUDE_TESTS)
+  add_subdirectory(utils/TableGen)
+endif()
 
 # Export CLANG_TABLEGEN_EXE for use by flang docs.
 set(CLANG_TABLEGEN_EXE "${CLANG_TABLEGEN_EXE}" CACHE INTERNAL "")

[rossburton]

I just updated the patch based on David's comments.

[DavidSpickett]

I will recuse myself from reviewing since I had input to the patch already, but want to highlight this that I said earlier:

In theory someone could be relying on there being another clang-tblgen in bin, maybe for installs, but that seems like a thing we can afford to break. They were building clang-tblgen anyway, so they could stop setting CLANG_TABLEGEN_EXE, and use the newly built one instead.

This is the main risk I see with doing this, and my guess of its impact may be totally wrong.

@rossburton could you add to the PR description the justification for the change, and the use case you want it for? Which I think is roughly - don't build clang-tblgen twice - it'll be used in some sort of build chain in Yocto.

[DavidSpickett]

Found a similar situation:

llvm-project/llvm/CMakeLists.txt

Line 1348 in 0db5ba0

if ( LLVM_INCLUDE_TESTS )

Not allowed to disable llvm utils and include llvm tests. Though in this case, 99% of tests would not run, rather than the maybe 0.01% you need clang-tblgen for.

[rossburton]

I updated the top comment, and can transfer the content to the patch if that would be useful (but didn't do it right now to avoid having the CI spin again).

[DavidSpickett]

I updated the top comment

Thank you, that's added the important context.

(but didn't do it right now to avoid having the CI spin again).

The final commit will use the description and title of this PR, rather than the git content. So you don't have to bother updating the commits in fact.