While fixing libMLIR.so I noticed a couple of implicit
dependency edges that happen to just work right now.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| mlir/lib/Analysis/CMakeLists.txt | ||
|---|---|---|
| 24 | This seems really annoying if necessary. There is already an explicit library for the dependency, e.g. MLIRCallInterfaces, this seems redundant and exposes things that shouldn't necessarily need to be exposed. | |
| mlir/lib/Analysis/CMakeLists.txt | ||
|---|---|---|
| 24 | Yeah it is really annoying... I had to add these for https://reviews.llvm.org/D78773 so it my be an issue with that PR. To reproduce on D78773, this should be enough cmake -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_TARGETS_TO_BUILD=host -DLLVM_ENABLE_PROJECTS=mlir -DLLVM_BUILD_LLVM_DYLIB:BOOL=ON ../llvm ninja | |
| mlir/lib/Analysis/CMakeLists.txt | ||
|---|---|---|
| 24 | @rriddle as far as I understand we need the DEPENDS directive to express a that target must be built (here generated headers) before you build the source for this library, while the target_link_librariesdependency only induces a link-time dependency. For example right now for me ninja clean && ninja tools/mlir/lib/Analysis/CMakeFiles/MLIRAnalysis.dir/CallGraph.cpp.o -nv | grep CallInterfaces shows that these dependency are already there. | |
| mlir/lib/Analysis/CMakeLists.txt | ||
|---|---|---|
| 24 | see comments on 78773. I think there's a simple way to fix it. The problem is actually that cmake needs special handling for dependencies on with generated files. (namely the .inc files) After the files are generated, it can see them and compute dependencies correctly using clang. However, before the files are generated it needs some extra implicit dependency to bootstrap the whole process. These extra dependencies seem to be a special case in cmake: only for PUBLIC dependencies which are library dependencies. D78771 partially breaks that because it enables object libraries, which results in another set of not-quite library targets which generate .o files for each .cpp file. These are included as regular sources, not library dependencies. | |
| mlir/lib/Analysis/CMakeLists.txt | ||
|---|---|---|
| 24 | I am missing some piece: since cmake runs entirely before the build start and does not re-run later, how can it "see" the generated files and compute dependencies using clang? | |
Comment Actions
@mehdi_amini cmake doesn't actually use clang, it apparently has it's own built-in preprocessor which scans for included files. In poking around through various previous answers about cmake, I think there are several things going on.
- in Ninja, cmake generates special order-only dependencies to resolve the bootstrapping of dependencies.
- There are some comments that this is more difficult with make because make isn't intended for an included file to be updated (potentially) multiple times. It's expected that there is a rule to generate an included dependence file once and then include it.
- There are other comments that dependencies are correctly handled if the custom command is in the same 'directory tree' as the libraries which include it. This may avoid the problems in 2 for simple cases.
I'm going to try to build a simple reproducer. Ultimately, the solution may be to just generate all the headers before building everything else, which means that all mlir libraries would have a dependence on all IncGen targets. That isn't great, but I think it's better than the fix proposed in this review.
Comment Actions
Ultimately, the solution may be to just generate all the headers before building everything else, which means that all mlir libraries would have a dependence on all IncGen targets. That isn't great, but I think it's better than the fix proposed in this review.
Yeah I was considering this as an alternative, something similar to intrinsics_gen in LLVM.
This seems really annoying if necessary. There is already an explicit library for the dependency, e.g. MLIRCallInterfaces, this seems redundant and exposes things that shouldn't necessarily need to be exposed.