pocl / pocl

pocl - Portable Computing Language
http://portablecl.org
MIT License
910 stars 251 forks source link

CMake code to find and link against OpenCL / ICD loader is problematic #1291

Open JayFoxRox opened 1 year ago

JayFoxRox commented 1 year ago

The current CMake setup is rather outdated and doesn't use the recommended way of integrating the OpenCL-ICD-Loader (which uses import targets now). Instead a couple of variables get created here:

https://github.com/pocl/pocl/blob/5a99e12d0bb78427ad948ac368589135507f1b59/CMakeLists.txt#L1022-L1023

However, OPENCL_LIBDIR is never actually used to control the linker, so if the OpenCL library is not part of the system library path, the build-script will fail to find it.

I think the build-system needs major redesign in this area.

To combat this, I'm currently using this patch:

diff --git a/bin/CMakeLists.txt b/bin/CMakeLists.txt
index 2fa37f44..f41983b1 100644
--- a/bin/CMakeLists.txt
+++ b/bin/CMakeLists.txt
@@ -28,7 +28,9 @@ set_opencl_header_includes()
 add_executable(poclcc poclcc.c)
 harden(poclcc)

-target_link_libraries(poclcc poclu ${OPENCL_LIBS})
+message("OPENCL_LIBS=${OPENCL_LIBS}")
+target_link_libraries(poclcc PRIVATE poclu ${OPENCL_LIBS})
+target_link_directories(poclcc PRIVATE "${OPENCL_LIBDIR}")

 install(TARGETS "poclcc" RUNTIME
         DESTINATION "${POCL_INSTALL_PUBLIC_BINDIR}" COMPONENT "poclcc")
diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt
index 2e2ddc0a..817f368a 100644
--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt
@@ -78,6 +78,8 @@ else()
   set(OPENCL_LIBS "${PTHREAD_LIBRARY};${POCL_LIBRARY_NAME};${POCL_TRANSITIVE_LIBS}")

 endif()
+message("OPENCL_LIBS=${OPENCL_LIBS} OPENCL_LIBDIR=${OPENCL_LIBDIR}")
+link_directories("${OPENCL_LIBDIR}")

 if(SANITIZER_OPTIONS)
   list(INSERT OPENCL_LIBS 0 ${SANITIZER_LIBS})
isuruf commented 1 year ago

Did you try the main branch or the release?

JayFoxRox commented 1 year ago

I'm on the main branch. The current main branch revision also still contains the problematic code linked above.

isuruf commented 1 year ago

It does have the unused code, but I believe https://github.com/pocl/pocl/pull/1252 fixes the issue that you are running into.

JayFoxRox commented 1 year ago

1252 fixes a different problem: It was about not being able to find the OpenCL ICD Loader package (= Bad OCL_ICD_LIBRARIES OCL_ICD_LIBDIR).

In my case OpenCL ICD Loader is found, so both of these variables are valid, but they don't get used correctly.

These variables get used to set OPENCL_LIBRARIES (= OCL_ICD_LIBRARIES) and OPENCL_LIBDIR and (= OCL_ICD_LIBDIR) then. Example: OPENCL_LIBS=Threads::Threads;OpenCL OPENCL_LIBDIR=/opt/homebrew/Cellar/opencl-icd-loader/2023.04.17/lib; both valid.

However, from that point onwards, only OPENCL_LIBRARIES is used. OPENCL_LIBDIR is used for some things, but the linker never gets told about it.

This means the linker links against -lOpenCL (from OPENCL_LIBRARIES) but it never adds the path where that lib can be found (should be from OPENCL_LIBDIR). So essentially missing -L${OPENCL_LIBDIR}.

A redesign with import targets would make this error impossible, as the library name, library path (and other potentially important things like flags) are part of the same "object".

franz commented 1 year ago

A redesign with import targets would make this error impossible

Indeed, most of the CMake code is old and doesn't use targets. However any redesign must also keep in mind that PoCL should also support other ICD loaders (Khronos), and also builds without ICD.