Page MenuHomePhabricator

[openmp] Use libffi only when LLVM_ENABLE_FFI is on
Needs RevisionPublic

Authored by twoh on Jun 3 2019, 11:18 PM.

Details

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.

Event Timeline

twoh created this revision.Jun 3 2019, 11:18 PM
Hahnfeld requested changes to this revision.Jun 3 2019, 11:26 PM

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.

This revision now requires changes to proceed.Jun 3 2019, 11:26 PM
twoh updated this revision to Diff 202864.Jun 3 2019, 11:47 PM

Addressed comments from @Hahnfeld. Thanks!

grokos added a comment.Jun 4 2019, 3:06 PM

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 updated this revision to Diff 203078.EditedJun 4 2019, 10:31 PM

@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

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 added a comment.Jun 4 2019, 11:28 PM

@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 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 added a comment.Jun 5 2019, 12:59 AM

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

Hahnfeld requested changes to this revision.Jul 9 2019, 8:17 AM

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

This revision now requires changes to proceed.Jul 9 2019, 8:17 AM