Page MenuHomePhabricator

Add dependency edges between generated headers and users
AbandonedPublic

Authored by vchuravy on Apr 23 2020, 4:25 PM.

Details

Summary

While fixing libMLIR.so I noticed a couple of implicit
dependency edges that happen to just work right now.

Diff Detail

Event Timeline

vchuravy created this revision.Apr 23 2020, 4:25 PM
rriddle added inline comments.Apr 23 2020, 4:28 PM
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.

vchuravy edited projects, added Restricted Project; removed Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2020, 4:42 PM
vchuravy marked an inline comment as done.Apr 23 2020, 4:46 PM
vchuravy added inline comments.
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
mehdi_amini added inline comments.Apr 23 2020, 9:35 PM
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.
That said different version of CMake (or different environments?) seem to behave differently on this, in the past @stephenneuendorffer observed different dependency than me.

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.

stephenneuendorffer requested changes to this revision.Apr 23 2020, 10:14 PM
stephenneuendorffer added inline comments.
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.

This revision now requires changes to proceed.Apr 23 2020, 10:14 PM
mehdi_amini added inline comments.Apr 23 2020, 10:49 PM
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?

@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.

  1. in Ninja, cmake generates special order-only dependencies to resolve the bootstrapping of dependencies.
  2. 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.
  3. 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.

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.

vchuravy abandoned this revision.May 4 2020, 6:40 AM

Abandoned in favour of @stephenneuendorffer recent list of changes.