[offload] Add missing build dependency

[jprotze]

libc++ headers must be generated before compiling part of liboffload.
The build error occurs if clang is configured to use libc++ by default.
Fixes issue #149324

[llvmbot]

@llvm/pr-subscribers-offload

Author: Joachim (jprotze)

Changes

libc++ headers must be generated before compiling part of liboffload.
The build error occurs if clang is configured to use libc++ by default.
Fixes issue #149324

---

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

1 Files Affected:

• (modified) offload/tools/offload-tblgen/CMakeLists.txt (+4)

diff --git a/offload/tools/offload-tblgen/CMakeLists.txt b/offload/tools/offload-tblgen/CMakeLists.txt
index 15525dc44ea60..64562fc72feac 100644
--- a/offload/tools/offload-tblgen/CMakeLists.txt
+++ b/offload/tools/offload-tblgen/CMakeLists.txt
@@ -22,5 +22,9 @@ add_tablegen(offload-tblgen OFFLOAD
   RecordTypes.hpp
   )
 
+if(TARGET cxx-headers)
+  add_dependencies(offload-tblgen cxx-headers)
+endif()
+
 set(OFFLOAD_TABLEGEN_EXE "${OFFLOAD_TABLEGEN_EXE}" CACHE INTERNAL "")
 set(OFFLOAD_TABLEGEN_TARGET "${OFFLOAD_TABLEGEN_TARGET}" CACHE INTERNAL "")

[callumfare]

This change looks fine, but is there a reason it only affects offload-tblgen? Could other offload components also run into this issue? I'm wondering if it's possible to capture the dependency at the level of the entire runtime when multiple are being built (e.g. openmp and offload might depend on libcxx)

[jprotze]

This change looks fine, but is there a reason it only affects offload-tblgen? Could other offload components also run into this issue? I'm wondering if it's possible to capture the dependency at the level of the entire runtime when multiple are being built (e.g. openmp and offload might depend on libcxx)

I was also a bit surprised, that adding this specific dependency fixes the build reproducibly (I did multiple clean builds with 100 processes). I think, that simply all other liboffload code depends on offload-tblgen ?

libomp is not affected, because we carefully avoid dependencies to a C++ runtime library.

[jhuber6]

So, this is for bootstapping with libcxx headers? It makes sense but i am wondering why it only shows up here.

[jprotze]

I'm not completely sure, what you mean with bootstrapping here, but I would say: No!

It affects building the whole llvm-project, even when using a llvm build from the same sources (built without offload). The problem is that for compiling the runtimes, we use the clang from the outer build directory, but it doesn't have the includes available at this point.

I tried to understand the dependencies in the ninja build file. I think that all other targets in offload have a dependency on some inc files that are processes with offload-tblgen and therefore have a dependency on offload-tblgen.

[jprotze]

I just tested, whether I can use a more general dependency and tried add_dependencies(LLVMOffload cxx-headers) + add_dependencies(omptarget cxx-headers) while removing the add_dependencies(offload-tblgen cxx-headers) dependency.
With that change, I get the same missing header issues as before.

Empirically, I would say this dependency is both necessary and sufficient.

[Meinersbur]

Since in a bootstrapping build the just-built Clang is being used, and offload-tblgen is writen in C++, requiring the libc++ includes to be builts before makes sense (unless Clang decides to use a a libstdc++ installation).

It looks like the entire tblgen setup will not work when building the runtimes for other targets than "default", but that's another issue.

[Meinersbur]

Could you add a comment for what this dependency is for?