It is all too easy to forget a CMake dependency on a TableGen’erated file. Often, this accidentally works out ok because Ninja happens to build things in an order where the dependency is satisfied out of pure luck. When it doesn’t, you get an error like:
llvm-project/llvm/include/llvm/IR/Attributes.h:91:14: fatal error: 'llvm/IR/Attributes.inc' file not found
#include "llvm/IR/Attributes.inc"
^~~~~~~~~~~~~~~~~~~~~~~~
I’d like to propose fixing that permanently.
What I have in mind is to add a guard to each generated file that complains if you are missing a preprocessor definition:
#ifndef TBLGEN_DEP_GUARD_ClangDriverOptions
# error If you're seeing this message, you're missing a dependency on ClangDriverOptions.
#endif
And then add target_compile_definitions(${target} INTERFACE -DTBLGEN_DEP_GUARD_${target}=1) under add_public_tablegen_target. Now the build will be broken if you’re missing the dependency, even if it might have accidentally succeeded otherwise, causing patch authors to “get it right” before they even land their change.
Now for the RFC:
Are there other ways we could solve this problem?
Is there anything we can do for Tablegen backends that emit outputs where preprocessor tricks like this are not appropriate (think JSON)?
Will this work for the non-CMake build system(s) (Bazel)?
That seems like a great idea! I don’t know why we haven’t thought of it sooner
Right now, I believe MLIR is having some sort of big funnel where “everything” on the consumer side depends on a meta “all Tablegen generated file” target, because maintaining the fine grain dependencies ended up not manageable.
This may or may not increase the burden maintenance for these systems… but should it be a factor? A condition for the inclusion of these was that they wouldn’t affect the project in any way.
I like your idea! I especially like that builds would be more deterministic. This is very valuable since most of these issues are ticking time bombs and buildbots will eventually hit them while not necessarily being able to point to an individual change (although if we’re lucky they will).
I also like that we’d be able to control the specific diagnostic. It’s not always obvious to most people (especially less-frequent contributors) why a #include may fail, so making it more obvious can help people understand and possibly fix it if they hit it.
To address the questions:
Are there other ways we could solve this problem?
I don’t think that there is a better way to address this with CMake. The generated files are already marked as GENERATED, but I don’t think CMake can automatically detect when a target depends on generated files without explicitly writing said dependency out with targets/files (e.g. depending on intrinsics_gen). We could also make it so everything always depends on all tablegen-related targets, but that would make incremental builds slower and more annoying…
Is there anything we can do for Tablegen backends that emit outputs where preprocessor tricks like this are not appropriate (think JSON)?
I’m not knowledgeable enough to consider tablegen backends that emit things like JSON. What kind of build failures do we see for these kinds of backends? The solution of “fail loudly if dependencies are incorrect” may translate to those somehow, but I’m not sure.
Will this work for the non-CMake build system(s) (Bazel)?
My guess is yes though I’m not 100% sure. My understanding is that people using these other build systems already take on the maintenance burden of doing so. I would consider this just another part of that burden.
Thanks for your proposal. It’s a good idea since everyone would notice his fault.
(I have been doing black magic to discover faults)
My random thoughts;
AFAIK add_custom_target doesn’t accept target_compile_definitions. It would be easy to rewrite using header-only library, though.
At least in clang, every ClangDriverOptions would be redundant since clangDriver provides ClangDriverOptions. We can enforce "An user of clang/Driver/Options.inc should depend on clangDriver, as far as clangDriver instantiates ClangDriverOptions.
You should suppress emitting the guard in local tblgen(s) (like tools/*/Options.td), since they are not targets but actions in add_custom_command.
I suggest;
Don’t put the guard in local tblgen.
Public generated headers should be instantiated by their implementations. For example, clangDriver should be responsible to generate ClangDriverOptions. (TODO: random headers in clangBasic?)
Put clangDriver rather than ClangDriverOptions as possible to satisfy deps.
Alternatively, you could put the .inc file in its own subdirectory and only add -Idir to CPPFLAGS when the dependency is present? Only need to touch the build system then, no need for TableGen changes, and would apply to other file formats too.
The -D approach and the -Idir approach have the same effect in this regard. With the former, we would do target_compile_definitions(${target} INTERFACE -DWHATEVER=1) and anything that builds against ${target} will need to have -DWHATEVER=1 set. The latter would be something like target_include_directories(${target} INTERFACE ${path_to_generated_inc}) and anything that builds against ${target} will need to include ${path_to_generated_inc}.
They both solve the problem of determinism in builds, so the tradeoff in my mind is controlling the diagnostic message versus limiting changes to tablegen.
Is the -I solution relying on unqualified include? It does not seem like good practice to me (risks of file name collision in particular)
See an include path in MLIR: llvm-project/MemRef.h at main · llvm/llvm-project · GitHub
The macro also has the same risk of collision though: but there may be a per-generator way to have something unique, for MLIR generators I can imagine a convention that would work for the macro I think.
I think that whether or not we use unqualified/qualified includes is independent of the solution itself. Using the example you gave, the line currently is #include "mlir/Dialect/MemRef/IR/MemRefOpsDialect.h.inc" and the file path is (presumably) something like ${build_dir}/include/mlir/Dialect/MemRef/IR/MemRefOpsDialect.h.inc.
An unqualified include solution would be to say that the generated file goes in ${build_dir}/include/mlir/Dialect/MemRef/IR/MemRefOps-generated/ (or something like this). Then in the code you’d do #include "MemRefOpsDialect.h.inc", and in CMake you’d do target_include_directories(${target} INTERFACE ${build_dir}/include/mlir/Dialect/MemRef/IR/MemRefOps-generated).
But with some adjustments, it could be made to be qualified like so: Generated file goes to the same place, but in the code you’d do #include "MemRefOps-generated/MemRefOpsDialect.h.inc" and in CMake you’d just do target_include_directories(${target} INTERFACE ${build_dir}/include/mlir/Dialect/MemRef/IR/).
I like that both the -I and the -D strategies are orthogonal to each other, and could be adopted independently or in unison. The former solves it for non-preprocessor-friendly outputs (JSON), and the latter has great story re: diagnostics.
Could we consider the impact to installation?
For the -D strategy, we could stuff the new CMake+Tablegen behavior under a CMake option. Perhaps defaulted to only be enabled when it’s a Debug build. Packagers can turn it off if it’s disruptive to their workflow. We can make it less disruptive by teaching llvm-config to emit the relevant -D flags.
To simplify things, I limited it to just clang/Driver/Options.inc. I’ll admit my cmake-fu is not strong enough to completely understand @chapuni’s suggestions, and I had to make some concessions:
does not support transitive dependencies
the relationship between the new tablegen flag and add_public_tablegen_target’s argument isn’t automatic
Suggestions and/or more concrete advice are extremely welcome.
Transitive usage is effective among add_library with target_link_libraries. In this point we have to get rid of add_custom_target and add_dependencies. The tree has grown up too larger to optimize deps to individual components.
I have tried locally the modification “partitioning generated headers” (with -I) and I am certain this would be less intrusive (but hairy in TableGen.cmake). Let me show my PoC diff in this weekend. (I won’t stop your followups!)
An unit for partitioning will be satisfied with the target name of header generator (eg. ClangDriverOptions) rather than individual tablegen() actions, since the target name should be unique in CMake’s target namespace.
The directory will be ${CMAKE_BINARY_DIR}/path/to/arbitrary/include/dir/ClangDriverOptions/clang/Driver. The additional include directory for target_include_directories may be ${CMAKE_BINARY_DIR}/path/to/arbitrary/include/dir/ClangDriverOptions.
The shortest form of /path/to/arbitrary/include/dir/${tablegen_target} is /ClangDriverOptions. Possibly longest form would be /tools/clang/include/clang/Driver/ClangDriverOptions. I think we could choose shorter but appropriate path as /include/ClangDriverOptions.
I think we should provide the cleaner to remove the old path include/clang/Driver/Options.inc at the configuration phase, to stabilize revision walking.
Modify migrating add_custom_target to add_library(INTERFACE) in add_public_tablegen_target. It required adopting install. I haven’t checked inspection for installed tree yet though.
Then, header files shall be provided as target_sources(PRIVATE) rather than target_sources(INTERFACE) due to CMake’s weird behavior.
I can provide target_include_directories(INTERFACE) to the isolated directory for the generated header.
clangDriver should be responsible to provide ClangDriverOptions. So users of clangDriver (with LINK_LIBRARIES) will make the generator header visible. In contrast, the build will fail in build time if a module doesn’t require clangDriver regardless of Options.inc.
${LLVM_GENERATED_INCLUDES_BASE}/path/to/file.inc is emitted to ${LLVM_GENERATED_INCLUDES_BASE}/${target}/path/to/file.inc with the include path ${LLVM_GENERATED_INCLUDES_BASE}/${target}.
I introduce “Header-only interface library” for it as ClangDriverOptions.
The component clangDriver exposes ClangDriverOptions as PUBLIC scope. So users of clangDriver can use clang/Driver/Options.inc.
In this diff, the execution of tablegen(...) defers to add_public_tablegen(target), since the output path of tablegen depends on the name of target in add_public_tablegen(target). I think we could have an option to exchange the order between tablegen and add_public_tablegen in such a case.
We would not rely on add_dependencies any more for tablegen action if we started using partitioning.
I know it would be more tough to apply changes to other tablegen files. I will investigate smarter ways for them.
@jroelofs I am sorry to create yet another diff rather than suggesting you changes. I thought it would be faster to show actual changes rather than pointing out changes. (and this is not an PoC using -D)
Prefacing with this not being a portable solution. I took a stab at working on this issue a few years ago using OS sandboxes:
The idea here is that we basically run all compile commands inside a sandbox that controls read access to tablegen output files based on the project dependency graph. It forces hard failures for any missing tablegen dependency.
I was never quite happy enough with the result but the proof of concept is fun.
