Clang modules builds depend header-wise on it as they ship all headers from the umbrella folders.

[v.g.vassilev]

Summary

Clang modules builds depend header-wise on it as they ship all headers from the umbrella folders. Building an entire module might include header, which depends on intrinsics_gen. Force target intrinsics_gen to be among the very first to build.

[v.g.vassilev]

Clang already depends on targets which depend on intrinsics_gen.

[beanz]

This patch adds a whole bunch of dependencies on intrinsics_gen that don't need to be there. I think you're approaching this problem from the wrong perspective.

In CMake you don't want to dictate the build order as much as the dependencies. You want the target build system (ninja, make, ...) to handle the order around the dependencies.

I'm not familiar with how module builds work, but it sounds to me like the module build needs to depend on intrinsics_gen, as opposed to what your patch does, which makes *everything* depend on intrinsics_gen.

[v.g.vassilev]

Thank you for the prompt review @beanz!

I agree with the comments. When compiling with modules, header A and B can be in the same module M. B depends on intrinsics_gen and A doesn't. When compiling a cpp file #include-ing header A, we implicitly request module M to be built. It puts header A and B in the same TU and tries to build them. Since B depends on intrinsics_gen (which might not be built yet) we run into issues such as: http://lab.llvm.org:8011/builders/clang-x86_64-linux-selfhost-modules/builds/17214/steps/compile.llvm.stage2/logs/stdio

I am not sure how to fix this issue. I have tried to solve it on modulemap level but failed. And anyway it would have been fragile (if the contents of the headers change we are back to square one). In a private discussion with Richard Smith we agreed that would be the least evil way to address the issue. An option would be to make this patch conditional if modules are enabled, but I personally don't like this approach because it makes the modules and non-modules builds different.

Can you recommend another acceptable way to enforce intrinsics_gen to be built first?

[beanz]

Module and non-module builds *are* different, and it sounds like they have different dependencies. Applying the intrinsics_gen dependency liberally around like your patch was doing will severely hurt build parallelism of non-module builds because it will force the intrinsics_gen bottleneck on more components. That is very undesirable to people not using modules.

If the best solution here is to add intrinsics_gen to LLVM_COMMON_DEPENDS, it should only be done if modules are being used. You also shouldn't need to remove all the optional_deps references from the CMakelists files because CMake should unique the dependencies, so duplicates will have no adverse impact.

[beanz]

Crazy idea... What if when modules are enabled we had CMake write out placeholder files for the tablegen-generataed headers? Then the module maps would be generated to include the inc files, and the module maps would be re-generated after intrinsics_gen when the headers change. Would that solve this problem?

[v.g.vassilev]

Yes IMO this would solve this issue but we need to build the module twice, once an incomplete one and the second time the complete one. I am not sure how would we regenerate the modulemaps to signal the change (eg. touch llvm/src/.../modulemap could work but IMO it is ugly).

It seems a bit more complex to implement.

[v.g.vassilev]

Force the dependency only if modules are enabled.

[v.g.vassilev]

Would that be a more acceptable change?

[beanz]

Yes, this is fine.

[v.g.vassilev]

Thanks! Landed in r274270.

CMakeLists.txt

 endif()
 
 # Put this before tblgen. Else we have a circular dependence.
 add_subdirectory(lib/Support)
 add_subdirectory(lib/TableGen)
 
 add_subdirectory(utils/TableGen)
 
+# Force target to be built as soon as possible. Clang modules builds depend
+# header-wise on it as they ship all headers from the umbrella folders. Building
+# an entire module might include header, which depends on intrinsics_gen. This
+# should be right after LLVMSupport and LLVMTableGen otherwise we introduce a
+# circular dependence.
+if (LLVM_ENABLE_MODULES)
+  list(APPEND LLVM_COMMON_DEPENDS intrinsics_gen)
+endif(LLVM_ENABLE_MODULES)
+
 add_subdirectory(include/llvm)
 
 add_subdirectory(lib)
 
 if( LLVM_INCLUDE_UTILS )
   add_subdirectory(utils/FileCheck)
   add_subdirectory(utils/PerfectShuffle)
   add_subdirectory(utils/count)