Fix missing build dependency on omp_gen.

[simon_tatham]

Summary

include/llvm/Frontend/OpenMP/CMakeLists.txt creates a new target
called omp_gen which builds the generated include file OMP.h.inc.
This target must therefore be a dependency of every compilation step
whose transitive #include dependencies contain OMP.h.inc, or else
it's possible for builds to fail if Ninja (or make or whatever)
schedules that compilation step before building OMP.h.inc at all.

A few of those dependencies are currently missing, which leads to
intermittent build failures, depending on the order that Ninja (or
whatever) happens to schedule its commands. As far as I can see,
compiles in clang/lib/CodeGen, clang/lib/Frontend, and
clang/examples all depend transitivily on OMP.h.inc (usually via
clang/AST/AST.h), but don't have the formal dependency in the ninja
graph.

Adding omp_gen to the dependencies of clang-tablegen-targets seems
to be the way to get the missing dependency into the clang/examples
subdirectory. This also fixes the other two clang subdirectories, as
far as I can see.

[clementval]

Sorry, I was not aware of this when I did the patch. Does it mean that some DEPENDS omp_gen added are not necessary anymore with this modification?

[simon_tatham]

I wondered about that. I think it may well mean some of those DEPENDS can be removed, but I'm not sure how to be certain of it :-)

[simon_tatham]

@clementval , are you happy for me to commit this patch?

[clementval]

Yeah I think it makes sense.

[simon_tatham]

Unfortunately, I had to revert this because it caused a buildbot failure: rG39ea5d74b283d5a42f34b856d22bfaf806a1c907

That puzzled me for a while, but I think I see the problem: llvm/cmake/modules/CMakeLists.txt calls configure_file to write out a fresh set of CMake files for modules to use, in which the main CMake system's definition of LLVM_COMMON_DEPENDS is exported, without necessarily exporting all the targets included in it.

modules/CMakeLists.txt includes code to remove the existing phony target intrinsics_gen from that variable before exporting it. So one fix would be to add omp_gen to the same code.

However, @clementval, do you think it would be a better idea to make your OpenMP tablegen command part of intrinsics_gen, along with all the rest of them, and remove omp_gen completely? That would make it work just like everything else. But perhaps you know more than I do about why that wouldn't work?

[clementval]

In D82659#2122505, @simon_tatham wrote:

Unfortunately, I had to revert this because it caused a buildbot failure: rG39ea5d74b283d5a42f34b856d22bfaf806a1c907

That puzzled me for a while, but I think I see the problem: llvm/cmake/modules/CMakeLists.txt calls configure_file to write out a fresh set of CMake files for modules to use, in which the main CMake system's definition of LLVM_COMMON_DEPENDS is exported, without necessarily exporting all the targets included in it.

modules/CMakeLists.txt includes code to remove the existing phony target intrinsics_gen from that variable before exporting it. So one fix would be to add omp_gen to the same code.

However, @clementval, do you think it would be a better idea to make your OpenMP tablegen command part of intrinsics_gen, along with all the rest of them, and remove omp_gen completely? That would make it work just like everything else. But perhaps you know more than I do about why that wouldn't work?

In D82659#2122505, @simon_tatham wrote:

Unfortunately, I had to revert this because it caused a buildbot failure: rG39ea5d74b283d5a42f34b856d22bfaf806a1c907

That puzzled me for a while, but I think I see the problem: llvm/cmake/modules/CMakeLists.txt calls configure_file to write out a fresh set of CMake files for modules to use, in which the main CMake system's definition of LLVM_COMMON_DEPENDS is exported, without necessarily exporting all the targets included in it.

modules/CMakeLists.txt includes code to remove the existing phony target intrinsics_gen from that variable before exporting it. So one fix would be to add omp_gen to the same code.

However, @clementval, do you think it would be a better idea to make your OpenMP tablegen command part of intrinsics_gen, along with all the rest of them, and remove omp_gen completely? That would make it work just like everything else. But perhaps you know more than I do about why that wouldn't work?

Since intrinsic_gen is used in many places, I'm not sure we want to pollute it with the omp_gen. Cannot you just add a depends for your failing case? I see many things depending on intrinsic_gen so it shouldn't harm to add a depend omp_gen where needed.

[simon_tatham]

Cannot you just add a depends for your failing case?

Perhaps. But the problem is that OMP.h.inc ends up being included from a lot of places – for example, anything that includes clang/AST/AST.h ultimately needs it. So the first problem in adding the missing dependencies on omp_gen is to find out where they all are, and that's not trivial.

In order to identify all the missing dependencies, I just wrote a small Python script that re-processes the output of ninja -t deps and ninja -t commands to find object files that can't be built without OMP.h.inc but don't have a formal dependency on a command that builds it.

That script showed that there are objects in clang/lib/CodeGen and clang/lib/Frontend that don't have this dependency, and also the extra awkward clang/examples/PrintFunctionNames, for which it's not even obvious where the extra dependency should be inserted.

But even if we put those in, it will guarantee that the build works now, but not that it will carry on working when someone adds a #include in a source directory that doesn't depend on omp_gen. So I don't think this really deserves the word "just"!

[clementval]

In D82659#2123127, @simon_tatham wrote:

Cannot you just add a depends for your failing case?

Perhaps. But the problem is that OMP.h.inc ends up being included from a lot of places – for example, anything that includes clang/AST/AST.h ultimately needs it. So the first problem in adding the missing dependencies on omp_gen is to find out where they all are, and that's not trivial.

In order to identify all the missing dependencies, I just wrote a small Python script that re-processes the output of ninja -t deps and ninja -t commands to find object files that can't be built without OMP.h.inc but don't have a formal dependency on a command that builds it.

That script showed that there are objects in clang/lib/CodeGen and clang/lib/Frontend that don't have this dependency, and also the extra awkward clang/examples/PrintFunctionNames, for which it's not even obvious where the extra dependency should be inserted.

But even if we put those in, it will guarantee that the build works now, but not that it will carry on working when someone adds a #include in a source directory that doesn't depend on omp_gen. So I don't think this really deserves the word "just"!

Ok, the word "just" is not a perfect fit here. Anyway, if omp_gen can be set up in a similar manner than intrinsics_gen it would probably be better than merging omp_gen into intrinsics_gen since they are different enough to have a separated target. intrinsics_gen is added as a dependency in a lot of place so I'm not sure it would solve the problem to add dependencies where needed.

[simon_tatham]

Here's a completely different patch, which adds all the missing dependencies on OMP.h.inc in the clang subdirectory in one go.

[clementval]

LGTM. So later the DEPENDS omp_gen that are in clang subdirectories could be removed right?

[clementval]

@simon_tatham Can you just update the description so it is consistent with your fix when it lands?

[simon_tatham]

[facepalm] Thank you. I carefully wrote a revised description, but forgot to upload it to this issue.

[simon_tatham]

In D82659#2125618, @clementval wrote:

LGTM. So later the DEPENDS omp_gen that are in clang subdirectories could be removed right?

That seems likely. I'm thinking what I should probably do is to polish up my checking script and try to get it into llvm/utils somewhere, and then it would be possible to make that kind of change and be confident that it wasn't removing a necessary dependency.

[jdoerfert]

Thank you both for figuring this out!

*Much* appreciated!

[simon_tatham]

@jdoerfert , @clementval : over in D83032 is a polished-up version of the script I used to check where the missing deps needed to go. Might be useful for the next problem of this kind. But I'm not sure who to get to review it; perhaps one of you might look at it?

[clementval]

In D82659#2130022, @simon_tatham wrote:

@jdoerfert , @clementval : over in D83032 is a polished-up version of the script I used to check where the missing deps needed to go. Might be useful for the next problem of this kind. But I'm not sure who to get to review it; perhaps one of you might look at it?

No idea who can review this.

[michele.scandale]

Why omp_gen is now a dependency of clang-tablegen-targets rather than being in the LLVM_COMMON_DEPENDS list like clang-tablegen-targets?

Moreover I've noticed that with the recent changes where omp_gen has been added as a dependency in several libraries, this was done unconditionally breaking the Clang standalone build.
For the same issue intrinsics_gen is added only if CLANG_BUILT_STANDALONE is false.

At this point I think that something like:

# All targets below may depend on all tablegen'd files.
get_property(CLANG_TABLEGEN_TARGETS GLOBAL PROPERTY CLANG_TABLEGEN_TARGETS)
add_custom_target(clang-tablegen-targets DEPENDS ${CLANG_TABLEGEN_TARGETS})
set_target_properties(clang-tablegen-targets PROPERTIES FOLDER "Misc")
list(APPEND LLVM_COMMON_DEPENDS clang-tablegen-targets)
if(NOT CLANG_BUILT_STANDALONE)
  list(APPEND LLVM_COMMON_DEPENDS omg_gen)
endif()

would fix all the issues, and it would allow removing the explicit dependencies added to each clang library.

Is there any issue with my reasoning?

[clementval]

In D82659#2136909, @michele.scandale wrote:

Why omp_gen is now a dependency of clang-tablegen-targets rather than being in the LLVM_COMMON_DEPENDS list like clang-tablegen-targets?

Moreover I've noticed that with the recent changes where omp_gen has been added as a dependency in several libraries, this was done unconditionally breaking the Clang standalone build.
For the same issue intrinsics_gen is added only if CLANG_BUILT_STANDALONE is false.

At this point I think that something like:

# All targets below may depend on all tablegen'd files.
get_property(CLANG_TABLEGEN_TARGETS GLOBAL PROPERTY CLANG_TABLEGEN_TARGETS)
add_custom_target(clang-tablegen-targets DEPENDS ${CLANG_TABLEGEN_TARGETS})
set_target_properties(clang-tablegen-targets PROPERTIES FOLDER "Misc")
list(APPEND LLVM_COMMON_DEPENDS clang-tablegen-targets)
if(NOT CLANG_BUILT_STANDALONE)
  list(APPEND LLVM_COMMON_DEPENDS omg_gen)
endif()

would fix all the issues, and it would allow removing the explicit dependencies added to each clang library.

Is there any issue with my reasoning?

Looks good but just one question ... When clang is built as standalone it does not build the OpenMP part inside Clang? I haven't seen any code to avoid compiling the OpenMP parsing and semantic checking inside clang.

[michele.scandale]

In D82659#2136999, @clementval wrote:

Looks good but just one question ... When clang is built as standalone it does not build the OpenMP part inside Clang? I haven't seen any code to avoid compiling the OpenMP parsing and semantic checking inside clang.

I don't think there is a way to avoid compiling the OpenMP support in Clang. The standalone build is just building the content of the clang directory as a separate CMake project reusing the an already built LLVM -- therefore the libLLVMFrontendOpenMP as well as the OMP.h.inc would have been generated already.

[clementval]

In D82659#2138228, @michele.scandale wrote:

In D82659#2136999, @clementval wrote:

Looks good but just one question ... When clang is built as standalone it does not build the OpenMP part inside Clang? I haven't seen any code to avoid compiling the OpenMP parsing and semantic checking inside clang.

I don't think there is a way to avoid compiling the OpenMP support in Clang. The standalone build is just building the content of the clang directory as a separate CMake project reusing the an already built LLVM -- therefore the libLLVMFrontendOpenMP as well as the OMP.h.inc would have been generated already.

Ok then your fix should work.

[michele.scandale]

Uhm.. it looks like it is not needed anymore. In the LLVMConfig.cmake that will be installed a intrinsics_gen and omp_gen custom targets are created for exactly the purpose of allowing out-of-tree or standalone builds to freely depend on them.
The Clang code where intrinsics_gen is conditionally added as a dependency is from 2014, while the change in LLVMConfig.cmake.in is from 2017.

llvm/include/llvm/CMakeLists.txt

 add_subdirectory(IR)
 add_subdirectory(Support)
 add_subdirectory(Frontend/OpenMP)
 
 # If we're doing an out-of-tree build, copy a module map for generated
 # header files into the build area.
 if (NOT "${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_BINARY_DIR}")
   configure_file(module.modulemap.build module.modulemap COPYONLY)
 endif (NOT "${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_BINARY_DIR}")
+
+set(LLVM_COMMON_DEPENDS ${LLVM_COMMON_DEPENDS} PARENT_SCOPE)

llvm/include/llvm/Frontend/OpenMP/CMakeLists.txt

 set(LLVM_TARGET_DEFINITIONS OMP.td)
 tablegen(LLVM OMP.h.inc --gen-directive-decls)
 add_public_tablegen_target(omp_gen)
+
+set(LLVM_COMMON_DEPENDS ${LLVM_COMMON_DEPENDS} PARENT_SCOPE)