[openmp] Use libffi only when LLVM_ENABLE_FFI is on

[twoh]

Summary

Currently libomptarget enables the use of libffi if the library is found, even when LLVM_ENABLE_FFI cmake variable is set to OFF. As this may confuse the users, guard libomptarget's use of libffi with LLVM_ENABLE_FFI.

[Hahnfeld]

We support standalone builds of openmp where LLVM_ENABLE_FFI will not be defined. Can you add a check for OPENMP_STANDALONE_BUILD OR or DEFINED LLVM_ENABLE_FFI? Also please try to be consistent with the surrounding style, ie no whitespace after the opening and before the closing parenthesis.

[twoh]

Addressed comments from @Hahnfeld. Thanks!

[grokos]

May I propose something extra? I guess the purpose of this patch is to disable libffi, for instance on systems where the library is not installed or for whatever reason the user doesn't want to use it. In that case, we should accommodate for the standalone build as well. We could introduce a new Cmake variable LIBOMPTARGET_ENABLE_FFI and

• set it to LLVM_ENABLE_FFI if building in-tree or
• have a default value of true if building standalone and let the user set it to false if they want to

Any thoughts?

[twoh]

@grokos Thanks for the suggestion. What about this?

• Off by default

cmake -G Ninja ../llvm-project/llvm -DLLVM_ENABLE_PROJECTS=openmp 2>&1 | grep LIBFFI
-- Could NOT find LIBOMPTARGET_DEP_LIBFFI (missing: LIBOMPTARGET_DEP_LIBFFI_LIBRARIES LIBOMPTARGET_DEP_LIBFFI_INCLUDE_DIRS)

• On if LIBOMPTARGET_ENABLE_FFI is set

cmake -G Ninja ../llvm-project/llvm -DLLVM_ENABLE_PROJECTS=openmp -DLIBOMPTARGET_ENABLE_FFI=ON 2>&1 | grep LIBFFI
-- Found LIBOMPTARGET_DEP_LIBFFI: /usr/lib64/libffi.so

• On if LLVM_ENABLE_FFI is on

cmake -G Ninja ../llvm-project/llvm -DLLVM_ENABLE_PROJECTS=openmp -DLLVM_ENABLE_FFI=ON 2>&1 | grep LIBFFI
-- Found LIBOMPTARGET_DEP_LIBFFI: /usr/lib64/libffi.so

• Option LIBOMPTARGET_ENABLE_FFI has a precedence over the default value

cmake -G Ninja ../llvm-project/llvm -DLLVM_ENABLE_PROJECTS=openmp -DLLVM_ENABLE_FFI=ON -DLIBOMPTARGET_ENABLE_FFI=OFF 2>&1 | grep LIBFFI
-- Could NOT find LIBOMPTARGET_DEP_LIBFFI (missing: LIBOMPTARGET_DEP_LIBFFI_LIBRARIES LIBOMPTARGET_DEP_LIBFFI_INCLUDE_DIRS)

• On by default if standalone build

cmake -G Ninja ../llvm-project/llvm -DLLVM_ENABLE_PROJECTS=openmp -DOPENMP_STANDALONE_BUILD=ON 2>&1 | grep LIBFFI
-- Found LIBOMPTARGET_DEP_LIBFFI: /usr/lib64/libffi.so

[Hahnfeld]

Oh, LLVM_ENABLE_FFI is off by default? That's not good, because we rely on the plugins to test libomptarget. So changing the default will have a negative impact on test coverage. I'm afraid that's not a good idea, even if we are now using libffi when LLVM_ENABLE_FFI = Off. We could still add LIBOMPTARGET_ENABLE_FFI (at the top-level CMakeLists.txt, please) and default to on, but I'm not sure if that's of much help.

[twoh]

@Hahnfeld I'm not an OpenMP expert and don't have much strong argument here, but IMHO it might be confusing to have LLVM_ENABLE_FFI and LIBOMPTARGET_ENABLE_FFI have different default value from user's perspective. I wonder if we can set it off by default and explicitly turn it on for builds with test?

[Hahnfeld]

In D62841#1530493, @twoh wrote:

@Hahnfeld I'm not an OpenMP expert and don't have much strong argument here, but IMHO it might be confusing to have LLVM_ENABLE_FFI and LIBOMPTARGET_ENABLE_FFI have different default value from user's perspective. I wonder if we can set it off by default and explicitly turn it on for builds with test?

Tests are enabled by default, so we'd also need to enable libffi by default.

[twoh]

@Hahnfeld Got it. Then it seems that there's not much we can do here :(

[Hahnfeld]

After some time has passed, I still don't see a convincing solution here. Maybe abandon the patch?

openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake

   
 ################################################################################
 # Looking for libffi...
 ################################################################################
 find_package(PkgConfig)
 
 pkg_check_modules(LIBOMPTARGET_SEARCH_LIBFFI QUIET libffi)
 
+if( LLVM_ENABLE_FFI )
   find_path (
     LIBOMPTARGET_DEP_LIBFFI_INCLUDE_DIR
     NAMES
       ffi.h
     HINTS
       ${LIBOMPTARGET_SEARCH_LIBFFI_INCLUDEDIR}
       ${LIBOMPTARGET_SEARCH_LIBFFI_INCLUDE_DIRS}
     PATHS
       /usr/include
       /usr/local/include
       /opt/local/include
       /sw/include
       ENV CPATH)
   
   # Don't bother look for the library if the header files were not found.
   if (LIBOMPTARGET_DEP_LIBFFI_INCLUDE_DIR)
     find_library (
         LIBOMPTARGET_DEP_LIBFFI_LIBRARIES
       NAMES
         ffi
       HINTS
         ${LIBOMPTARGET_SEARCH_LIBFFI_LIBDIR}
         ${LIBOMPTARGET_SEARCH_LIBFFI_LIBRARY_DIRS}
       PATHS
         /usr/lib
         /usr/local/lib
         /opt/local/lib
         /sw/lib
         ENV LIBRARY_PATH
         ENV LD_LIBRARY_PATH)
   endif()
+endif( LLVM_ENABLE_FFI )
 
 set(LIBOMPTARGET_DEP_LIBFFI_INCLUDE_DIRS ${LIBOMPTARGET_DEP_LIBFFI_INCLUDE_DIR})
 find_package_handle_standard_args(
   LIBOMPTARGET_DEP_LIBFFI 
   DEFAULT_MSG
   LIBOMPTARGET_DEP_LIBFFI_LIBRARIES
   LIBOMPTARGET_DEP_LIBFFI_INCLUDE_DIRS)