icl-utk-edu / heffte

BSD 3-Clause "New" or "Revised" License
20 stars 15 forks source link

Linking error with fftw3_omp #46

Closed ahojukka5 closed 2 months ago

ahojukka5 commented 3 months ago

There is an issue with compiling software on LUMI.

module load LUMI/23.09 partition/C
module load cray-python cray-fftw
cmake -S heffte-2.4.0-src -B heffte-2.4.0-build -DCMAKE_BUILD_TYPE=Release -D Heffte_ENABLE_AVX=ON -D Heffte_ENABLE_FFTW=ON -D Heffte_ENABLE_PYTHON=ON -D CMAKE_CXX_COMPILER=CC -D MPI_CXX_COMPILER=CC
[  1%] Building CXX object CMakeFiles/Heffte.dir/src/heffte_c.cpp.o
[  3%] Building CXX object CMakeFiles/Heffte.dir/src/heffte_plan_logic.cpp.o
[  5%] Building CXX object CMakeFiles/Heffte.dir/src/heffte_magma_helpers.cpp.o
[  6%] Building CXX object CMakeFiles/Heffte.dir/src/heffte_reshape3d.cpp.o
[  8%] Building CXX object CMakeFiles/Heffte.dir/src/heffte_compute_transform.cpp.o
[ 10%] Linking CXX shared library libheffte.so
[ 10%] Built target Heffte
[ 12%] Building CXX object benchmarks/CMakeFiles/convolution.dir/convolution.cpp.o
[ 13%] Linking CXX executable convolution
ld.lld: error: undefined reference due to --no-allow-shlib-undefined: omp_get_thread_num
>>> referenced by /opt/cray/pe/fftw/3.3.10.5/x86_milan/lib/libfftw3_omp.so

ld.lld: error: undefined reference due to --no-allow-shlib-undefined: omp_get_num_threads
>>> referenced by /opt/cray/pe/fftw/3.3.10.5/x86_milan/lib/libfftw3_omp.so

ld.lld: error: undefined reference due to --no-allow-shlib-undefined: GOMP_parallel
>>> referenced by /opt/cray/pe/fftw/3.3.10.5/x86_milan/lib/libfftw3_omp.so

ld.lld: error: undefined reference due to --no-allow-shlib-undefined: omp_get_thread_num
>>> referenced by /opt/cray/pe/fftw/3.3.10.5/x86_milan/lib/libfftw3f_omp.so

ld.lld: error: undefined reference due to --no-allow-shlib-undefined: omp_get_num_threads
>>> referenced by /opt/cray/pe/fftw/3.3.10.5/x86_milan/lib/libfftw3f_omp.so

With a small modification to FindHeffteFFTW.cmake the compilation is working:

# respect user provided FFTW_LIBRARIES
if (NOT FFTW_LIBRARIES)
    heffte_find_fftw_libraries(
        PREFIX ${FFTW_ROOT}
        VAR FFTW_LIBRARIES
        REQUIRED "fftw3" "fftw3f"
        OPTIONAL "fftw3_omp" "fftw3f_omp" "fftw3_threads" "fftw3f_threads"
                               )
#   if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
#       find_package(OpenMP REQUIRED)
#       list(APPEND FFTW_LIBRARIES ${OpenMP_CXX_LIBRARIES})
#   else()
#       if ("fftw3_omp" IN_LIST FFTW_LIBRARIES)
#           list(APPEND FFTW_LIBRARIES "-lgomp")
#       endif()
#   endif()

    find_package(OpenMP REQUIRED)
    if (OpenMP_FOUND)
        list(APPEND FFTW_LIBRARIES OpenMP::OpenMP_CXX)
    endif()

endif()

Before submitting PR, I'd like to understand the rationale behind the if-else and whether it should be fixed in some other way..?

mkstoyanov commented 3 months ago

Thanks for checking in, I do think there's more here than meets the eye.

The main challenge is to correctly capture the FFTW dependencies since FFTW does not come with it's own CMake export file.

The code is making the assumption that the OpenMP variant of FFTW is build with GNU OpenMP which is certainly the case if we use a standard Linux distribution (e.g., Ubuntu) or the majority of systems that we have tested. In that case, we must link to the GNU version of the OpenMP libraries. If we are also using the GNU compiler we will simply enable OpenMP with the find_package(). If we are using a non-gnu compiler, e.g., ROCm clang compiler, then we add the -lgomp flag. Your fix will break that use case since enabling OpenMP for clang will use the Intel libiomp symbols while FFTW will require the libgomp symbols.

From you log that you posted, I don't see which compiler is used on your system. The fix may be better if you just use:

if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU" OR ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "<insert your compiler here>")

In addition and without making any changes to heFFTe, you can manually specify the required set of FFTW libraries, e.g., some rough approximation of this line:

cmake ... -DFFTW_LIBRARIES="/path/lib/libfftw3.so;/path/lib/libfftw3f.so;/path/lib/libfftw3_omp.so;/path/lib/libomp.so;"

You can use the ldd Linux tool to find which libraries are needed as dependencies by a specific shared library:

ldd /path/lib/libfftw3_omp.so
ahojukka5 commented 3 months ago

The compiler used is

Cray clang version 16.0.1  (6d4824324d375100ba18ca639dfc956fe6546d06)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /opt/cray/pe/cce/16.0.1/cce-clang/x86_64/share/../bin

After adding some debugging messages, CMAKE_CXX_COMPILER_ID gets the value GNU. So we are indeed in the first branch and do find_package(OpenMP REQUIRED). However, OpenMP_CXX_LIBRARIES = /usr/lib64/gcc/x86_64-suse-linux/7/libgomp.so;/usr/lib64/libpthread.so which is wrong. Another way to get around this would be to change the single line:

--- a/cmake/FindHeffteFFTW.cmake.orig   2024-06-10 18:37:51.000000000 +0300
+++ b/cmake/FindHeffteFFTW.cmake    2024-06-11 14:19:13.000000000 +0300
@@ -77,7 +77,7 @@
                                )
     if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
         find_package(OpenMP REQUIRED)
-        list(APPEND FFTW_LIBRARIES ${OpenMP_CXX_LIBRARIES})
+        list(APPEND FFTW_LIBRARIES OpenMP::OpenMP_CXX)
     else()
         if ("fftw3_omp" IN_LIST FFTW_LIBRARIES)
             list(APPEND FFTW_LIBRARIES "-lgomp")

So it will add OpenMP::OpenMP_CXX to the end of FFTW_LIBRARIES. Would this make more sense?

mkstoyanov commented 3 months ago

Yes, this is the correct approach. I should have probably done this myself back in the day.

The change needs to be synchronized with cmake/HeffteConfig.cmake which needs something to the effect:

if (@Heffte_ENABLE_FFTW@ AND NOT TARGET Heffte::FFTW)
    add_library(Heffte::FFTW INTERFACE IMPORTED GLOBAL)
    if (@OpenMP_FOUND@)
        find_package(OpenMP REQUIRED)
    endif()
    target_link_libraries(Heffte::FFTW INTERFACE @FFTW_LIBRARIES@)
    set_target_properties(Heffte::FFTW PROPERTIES INTERFACE_INCLUDE_DIRECTORIES @FFTW_INCLUDES@)
endif()

You can make a PR for one or both changes or I can make a PR myself. Your choice.