Page MenuHomePhabricator

[OpenMP] Not set OPENMP_STANDALONE_BUILD=ON when building OpenMP along with LLVM
ClosedPublic

Authored by tianshilei1992 on Dec 22 2020, 5:32 PM.

Details

Summary

For now, *_STANDALONE_BUILD is set to ON even if they're built along
with LLVM because of issues mentioned in the comments. This can cause some issues.
For example, if we build OpenMP along with LLVM, we'd like to copy those OpenMP
headers to <prefix>/lib/clang/<version>/include such that clang can find
those headers without using -I <prefix>/include because those headers will be
copied to <prefix>/include if it is built standalone.

In this patch, we fixed the dependence issue in OpenMP such that it can be built
correctly even with OPENMP_STANDALONE_BUILD=OFF. The issue is in the call to
add_lit_testsuite, where clang and clang-resource-headers are passed as
DEPENDS. Since we're building OpenMP along with LLVM, clang is set by CMake
to be the C/C++ compiler, therefore these two dependences are no longer needed,
where caused the dependence issue.

Diff Detail

Event Timeline

tianshilei1992 created this revision.Dec 22 2020, 5:32 PM
tianshilei1992 requested review of this revision.Dec 22 2020, 5:32 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript

If I build llvm-project using an old compiler, the dependencies are necessary in order to have clang built before running the OpenMP tests. Dropping the dependencies might cause issues in parallel builds, but will make the check-openmp targets incomplete.
What is the problem in having the dependency there?

llvm/runtimes/CMakeLists.txt
130

Are you sure that openmp is in ${runtimes}?

openmp/cmake/OpenMPTesting.cmake
188–189

The dependency allows to run ninja check-libomp in a newly configured BUILD-dir.
When you drop this dependency, clang will not be built and the testing will fail.

If I build llvm-project using an old compiler, the dependencies are necessary in order to have clang built before running the OpenMP tests. Dropping the dependencies might cause issues in parallel builds, but will make the check-openmp targets incomplete.
What is the problem in having the dependency there?

Building runtimes is not part of building LLVM. It is different from building OpenMP using CMake argument LLVM_ENABLE_PROJECTS . Instead, the CMake command for those runtimes will be invoked *after* building LLVM, so it's a two-pass compilation. In the second run, in addition to some specific CMake arguments to be passed through, one of the most important thing is to set C/C++ compiler to the clang built in the first run. Therefore, all CMake targets generated in the first run will not be valid in the second run. As a result, CMake raises an error that:

  • The dependency target "clang" of target XXX does not exist.
  • The dependency target "clang-resource-headers" of target XXX does not exist.

However, since we already know that C/C++ compiler for the runtimes is already set to clang explicitly, we can drop this dependences safely.

LLVM_ENABLE_PROJECTS says building those projects with toolchain same as building LLVM, which is actually a standalone build.

The reason the two dependent targets are missing is that clang is not imported to CMake in the second run. There is a find_package(LLVM) but no find_package(Clang). So if I add that missing package, clang will be resolved. However, clang-resource-headers is still missing because in the CMake module of Clang, clang-resource-headers is not exported. IMO, clang-resource-headers is not important here at all because once we have clang, clang can always find those headers as they're in the hard-coded path, so it's kind of redundant to have both clang and clang-resource-headers here. Like I said before, even clang is not needed because we already set the C/C++ compiler to clang.

Building runtimes is not part of building LLVM. It is different from building OpenMP using CMake argument LLVM_ENABLE_PROJECTS .

It might be different, but your change still breaks the OpenMP tests, when OpenMP is built using LLVM_ENABLE_PROJECTS.

Can you test for the clang target and only set the dependency, if the target exists in the current configuration?

tianshilei1992 added a comment.EditedJan 4 2021, 7:11 PM

Building runtimes is not part of building LLVM. It is different from building OpenMP using CMake argument LLVM_ENABLE_PROJECTS .

It might be different, but your change still breaks the OpenMP tests, when OpenMP is built using LLVM_ENABLE_PROJECTS.

Can you test for the clang target and only set the dependency, if the target exists in the current configuration?

I think I got your point. You were saying that when we don't have any build, and just execute ninja check-libomp, then the two dependent targets will not be built. You're right. I'll fix this issue.

Fixed the issue in the dependence

@protze.joachim This looks good to me, OK with you?

LGTM, and should work for the runtime tests.

I could not figure out, why clang-resource-headers was there in the first place. As I understand clang-resource-headers, it's just an install target?
Also, the relevant files behind this target seem to be target-specific wrappers of headers like math.h and new. They might be relevant for some libomptarget tests? But, grep didn't show any use of math headers in libomptarget tests.

Removed useless comment

tianshilei1992 marked 2 inline comments as done.Jan 9 2021, 10:10 AM
tianshilei1992 added inline comments.
llvm/runtimes/CMakeLists.txt
130

Adding openmp to LLVM_ALL_RUNTIMES could be in another patch.

jdoerfert accepted this revision.Jan 10 2021, 1:42 PM

LGTM, and should work for the runtime tests.

I could not figure out, why clang-resource-headers was there in the first place. As I understand clang-resource-headers, it's just an install target?
Also, the relevant files behind this target seem to be target-specific wrappers of headers like math.h and new. They might be relevant for some libomptarget tests? But, grep didn't show any use of math headers in libomptarget tests.

If they are needed, we'll figure it out. So far it looks they are not. LGTM.

This revision is now accepted and ready to land.Jan 10 2021, 1:42 PM