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

[tianshilei1992]

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.

[protze.joachim]

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

129

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.

[tianshilei1992]

In D93738#2476488, @protze.joachim wrote:

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.

[tianshilei1992]

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.

[protze.joachim]

In D93738#2476692, @tianshilei1992 wrote:

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]

In D93738#2476961, @protze.joachim wrote:

In D93738#2476692, @tianshilei1992 wrote:

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.

[tianshilei1992]

Fixed the issue in the dependence

[jdoerfert]

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

[protze.joachim]

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.

[tianshilei1992]

Removed useless comment

[tianshilei1992]

llvm/runtimes/CMakeLists.txt

129

Adding openmp to LLVM_ALL_RUNTIMES could be in another patch.

[jdoerfert]

In D93738#2484324, @protze.joachim wrote:

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.

llvm/runtimes/CMakeLists.txt

if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_SOURCE_DIR})
   # Use libtool instead of ar if you are both on an Apple host, and targeting Apple.
   if(CMAKE_HOST_APPLE AND APPLE)
     include(UseLibtool)
   endif()
 
   # This can be used to detect whether we're in the runtimes build.
   set(RUNTIMES_BUILD ON)
 
   foreach(entry ${runtimes})
     get_filename_component(projName ${entry} NAME)
 
     # TODO: Clean this up as part of an interface standardization
     string(REPLACE "-" "_" canon_name ${projName})
     string(TOUPPER ${canon_name} canon_name)
 
     # The subdirectories need to treat this as standalone builds. D57992 tried
     # to get rid of this, but the runtimes treat *_STANDALONE_BUILD=OFF as if
     # llvm & clang are configured in the same CMake, and setup dependencies
-    # against their targets.
+    # against their targets. OpenMP has fixed the issue so we don't set the
+    # variable.
+    if (NOT ${canon_name} STREQUAL "OPENMP")
       set(${canon_name}_STANDALONE_BUILD ON)
+    endif()
 
     if(LLVM_RUNTIMES_LIBDIR_SUBDIR)
       set(${canon_name}_LIBDIR_SUBDIR "${LLVM_RUNTIMES_LIBDIR_SUBDIR}" CACHE STRING "" FORCE)
     endif()
 
     # Setting a variable to let sub-projects detect which other projects
     # will be included under here.
     set(HAVE_${canon_name} ON)

openmp/cmake/OpenMPTesting.cmake

if (${OPENMP_STANDALONE_BUILD})
     set(LIT_ARGS ${OPENMP_LIT_ARGS} ${ARG_ARGS})
     add_custom_target(${target}
       COMMAND ${PYTHON_EXECUTABLE} ${OPENMP_LLVM_LIT_EXECUTABLE} ${LIT_ARGS} ${ARG_UNPARSED_ARGUMENTS}
       COMMENT ${comment}
       DEPENDS ${ARG_DEPENDS}
       USES_TERMINAL
     )
   else()
+    # Since we're building along with LLVM, clang and clang-resource-headers are
+    # guaranteed so we don't need them in DEPENDS, which can cause issues.
     if (ARG_EXCLUDE_FROM_CHECK_ALL)
       add_lit_testsuite(${target}
         ${comment}
         ${ARG_UNPARSED_ARGUMENTS}
         EXCLUDE_FROM_CHECK_ALL
-        DEPENDS clang clang-resource-headers FileCheck ${ARG_DEPENDS}
+        DEPENDS FileCheck ${ARG_DEPENDS}
         ARGS ${ARG_ARGS}
       )
     else()
       add_lit_testsuite(${target}
         ${comment}
         ${ARG_UNPARSED_ARGUMENTS}
-        DEPENDS clang clang-resource-headers FileCheck ${ARG_DEPENDS}
+        DEPENDS FileCheck ${ARG_DEPENDS}
         ARGS ${ARG_ARGS}
       )
     endif()
   endif()
 endfunction()
 
 function(construct_check_openmp_target)
   get_property(OPENMP_LIT_TESTSUITES GLOBAL PROPERTY OPENMP_LIT_TESTSUITES)
   get_property(OPENMP_LIT_DEPENDS GLOBAL PROPERTY OPENMP_LIT_DEPENDS)
 
   # We already added the testsuites themselves, no need to do that again.
   add_openmp_testsuite(check-openmp "Running OpenMP tests" ${OPENMP_LIT_TESTSUITES} EXCLUDE_FROM_CHECK_ALL DEPENDS ${OPENMP_LIT_DEPENDS})
 endfunction()