Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake: Support building more than one precision #1

Merged
merged 2 commits into from
Nov 21, 2017
Merged

CMake: Support building more than one precision #1

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b423d9
CMake: Support building more than one precision
lukas-w Nov 21, 2017
ac2ad83
Minor CMake improvements
lukas-w Nov 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
253 changes: 134 additions & 119 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ option (ENABLE_THREADS "Use pthread for multithreading" OFF)
option (WITH_COMBINED_THREADS "Merge thread library" OFF)

option (ENABLE_FLOAT "single-precision" OFF)
option (ENABLE_DOUBLE "double-precision" ON)
option (ENABLE_LONG_DOUBLE "long-double precision" OFF)
option (ENABLE_QUAD_PRECISION "quadruple-precision" OFF)

Expand Down Expand Up @@ -263,145 +264,159 @@ endif ()

set (FFTW_VERSION 3.3.7)

set (PREC_SUFFIX)
if (ENABLE_FLOAT)
set (FFTW_SINGLE TRUE)
set (BENCHFFT_SINGLE TRUE)
set (PREC_SUFFIX f)
if (BUILD_SHARED_LIBS)
add_definitions (-DFFTW_DLL)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GenerateExportHeader should be used instead:

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is code I kept from the original CMakeLists.txt, so I'm hesitant to change it as it's not really related to the issue this PR is addressing. Or would you argue the change is necessary for clean use within Hunter?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand this code will be used in installed headers too (can't check because I can't install). Meaning the burden of setting FFTW_DLL moved to user. GenerateExportHeader designed to resolve all problems.

Or would you argue the change is necessary for clean use within Hunter?

I'm pretty sure you we will hit this problem while testing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for explaining. I tested this PR on Windows and didn't have any problems regarding export symbols, so I think it's fine this way. ffftw.h lines 78 to 87 handles this fine as far as I can see:

#if defined(FFTW_DLL) && (defined(_WIN32) || defined(__WIN32__))
   /* annoying Windows syntax for shared-library declarations */
#  if defined(COMPILING_FFTW) /* defined in api.h when compiling FFTW */
#    define FFTW_EXTERN extern __declspec(dllexport)
#  else /* user is calling FFTW; import symbol */
#    define FFTW_EXTERN extern __declspec(dllimport)
#  endif
#else
#  define FFTW_EXTERN extern
#endif

I can do some further testing. If it turns out not to be sufficient, I'll open a separate PR upstream.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried to install it and add it to user's project with find_package(fftw3 CONFIG REQUIRED)?

Yes, I successfully built and ran a project that depends on FFTW3 using Hunter on MSVC, so I did

hunter_add_package(fftw3)
find_package(fftw3f REQUIRED)

I have the Hunter patches on my local machine and will submit them once this PR is settled.

FYI, I'm a developer on the LMMS project (https://github.com/LMMS/lmms). Adding FFTW3 (and some other packages) to Hunter is part of efforts to make LMMS compatible with MSVC. We plan on using Hunter to fetch dependencies on Windows.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, upstream still supports the autotools build system, so I don't think a GenerateExportHeader patch would be accepted because it could render the two systems incompatible.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hunter_add_package(fftw3)
find_package(fftw3f REQUIRED)

It should be with CONFIG:

hunter_add_package(fftw3)
find_package(fftw3f CONFIG REQUIRED)

Note that my original question was about shared library (DLL on Windows). Hunter by default build static library, you have to add CMAKE_ARGS BUILD_SHARED_LIBS=ON:

As a summary it's up to you. I'm just pretty sure this will lead to opaque linker error on user's side.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be with CONFIG:

Using CONFIG will break compatibility with non-Hunter builds.

Hunter by default build static library

Ah, I wasn't aware of that. I tried using BUILD_SHARED_LIBS=ON but didn't get any linking errors. Anyway, I'll test this on more environments in the context of LMMS and will submit another patch in case I run into problems.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using CONFIG will break compatibility with non-Hunter builds

I think you're already using CONFIG implicilty.

endif ()

if (ENABLE_LONG_DOUBLE)
set (FFTW_LDOUBLE TRUE)
set (BENCHFFT_LDOUBLE TRUE)
set (PREC_SUFFIX l)
endif ()
function (add_fftw_library PRECISION)
if (PRECISION MATCHES "SINGLE|LDOUBLE|QUAD")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use MATCHES "^(SINGLE|LDOUBLE|QUAD)$")

set (FFTW_${PRECISION} TRUE)
set (BENCHFFT_${PRECISION} TRUE)
if (PRECISION STREQUAL SINGLE)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should update requirement to CMake 3.1+ or use string(COMPARE EQUAL ...):

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This problem looks like it will affect the if (PRECISION MATCHES "...") line too, so I guess it's better to update to 3.1.

set (PREC_SUFFIX f)
else ()
string (SUBSTRING ${PRECISION} 0 1 PREC_SUFFIX)
string (TOLOWER ${PREC_SUFFIX} PREC_SUFFIX)
endif()
endif ()

if (ENABLE_QUAD_PRECISION)
set (FFTW_QUAD TRUE)
set (BENCHFFT_QUAD TRUE)
set (PREC_SUFFIX q)
endif ()
set (fftw3_lib fftw3${PREC_SUFFIX})
set (fftw3_lib fftw3${PREC_SUFFIX})

configure_file (cmake.config.h.in config.h @ONLY)
include_directories (${CMAKE_CURRENT_BINARY_DIR})
add_library (${fftw3_lib} ${SOURCEFILES})

if (BUILD_SHARED_LIBS)
add_definitions (-DFFTW_DLL)
endif ()
configure_file (cmake.config.h.in ${CMAKE_CURRENT_BINARY_DIR}/${fftw3_lib}/config.h @ONLY)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've changed location of config.h file but I see no change of C++ code. Is it correct?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the target_include_directories in the line below accounts for this change, so that #include "config.h" still works.

target_include_directories (${fftw3_lib} PRIVATE "${CMAKE_CURRENT_BINARY_DIR}/${fftw3_lib}")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use $<BUILD_INTERFACE:...> here:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is using PRIVATE, so $<BUILD_INTERFACE:...> won't make a difference here, as PRIVATE includes are only ever used during build, right?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, okay.


if (MSVC)
target_compile_definitions (${fftw3_lib} PRIVATE /bigobj)
endif ()
if (HAVE_SSE)
target_compile_options (${fftw3_lib} PRIVATE ${SSE_FLAG})
endif ()
if (HAVE_SSE2)
target_compile_options (${fftw3_lib} PRIVATE ${SSE2_FLAG})
endif ()
if (HAVE_AVX)
target_compile_options (${fftw3_lib} PRIVATE ${AVX_FLAG})
endif ()
if (HAVE_AVX2)
target_compile_options (${fftw3_lib} PRIVATE ${AVX2_FLAG})
endif ()
if (HAVE_FMA)
target_compile_options (${fftw3_lib} PRIVATE ${FMA_FLAG})
endif ()
if (HAVE_LIBM)
target_link_libraries (${fftw3_lib} m)
endif ()

add_library (${fftw3_lib} ${SOURCEFILES})
if (MSVC)
target_compile_definitions (${fftw3_lib} PRIVATE /bigobj)
endif ()
if (HAVE_SSE)
target_compile_options (${fftw3_lib} PRIVATE ${SSE_FLAG})
endif ()
if (HAVE_SSE2)
target_compile_options (${fftw3_lib} PRIVATE ${SSE2_FLAG})
endif ()
if (HAVE_AVX)
target_compile_options (${fftw3_lib} PRIVATE ${AVX_FLAG})
endif ()
if (HAVE_AVX2)
target_compile_options (${fftw3_lib} PRIVATE ${AVX2_FLAG})
endif ()
if (HAVE_FMA)
target_compile_options (${fftw3_lib} PRIVATE ${FMA_FLAG})
endif ()
if (HAVE_LIBM)
target_link_libraries (${fftw3_lib} m)
endif ()
list (APPEND subtargets ${fftw3_lib})

set (subtargets ${fftw3_lib})
if (ENABLE_THREADS AND Threads_FOUND)
if (WITH_COMBINED_THREADS)
target_link_libraries (${fftw3_lib} ${CMAKE_THREAD_LIBS_INIT})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Threads::Threads available since CMake 3.1:

Use it intead of CMAKE_THREAD_LIBS_INIT.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concerns as with GenerateExportHeader. Also, this will bump the minimum CMake version up to 3.1. I'm not sure if this is desired upstream. Ultimately, we'll want to be in sync with upstream so we can drop this fork, right? I can open a separate PR upstream though and see what they say. :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately, we'll want to be in sync with upstream so we can drop this fork, right?

Also we want everything to work correctly. If it will not be accepted upstream, then just use fork.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I'll change it o use Threads::Threads as we'll update to CMake 3.1 anyway because of CMP0054.

else ()
add_library (${fftw3_lib}_threads ${fftw_threads_SOURCE})
target_link_libraries (${fftw3_lib}_threads ${fftw3_lib})
target_link_libraries (${fftw3_lib}_threads ${CMAKE_THREAD_LIBS_INIT})
list (APPEND subtargets ${fftw3_lib}_threads)
endif ()
endif ()

if (Threads_FOUND)
if (WITH_COMBINED_THREADS)
target_link_libraries (${fftw3_lib} ${CMAKE_THREAD_LIBS_INIT})
else ()
add_library (${fftw3_lib}_threads ${fftw_threads_SOURCE})
target_link_libraries (${fftw3_lib}_threads ${fftw3_lib})
target_link_libraries (${fftw3_lib}_threads ${CMAKE_THREAD_LIBS_INIT})
list (APPEND subtargets ${fftw3_lib}_threads)
if (ENABLE_OPENMP AND OPENMP_FOUND)
add_library (${fftw3_lib}_omp ${fftw_omp_SOURCE})
target_link_libraries (${fftw3_lib}_omp ${fftw3_lib})
target_link_libraries (${fftw3_lib}_omp ${CMAKE_THREAD_LIBS_INIT})
list (APPEND subtargets ${fftw3_lib}_omp)
target_compile_options (${fftw3_lib}_omp PRIVATE ${OpenMP_C_FLAGS})
endif ()
endif ()

if (OPENMP_FOUND)
add_library (${fftw3_lib}_omp ${fftw_omp_SOURCE})
target_link_libraries (${fftw3_lib}_omp ${fftw3_lib})
target_link_libraries (${fftw3_lib}_omp ${CMAKE_THREAD_LIBS_INIT})
list (APPEND subtargets ${fftw3_lib}_omp)
target_compile_options (${fftw3_lib}_omp PRIVATE ${OpenMP_C_FLAGS})
install(TARGETS ${fftw3_lib}
EXPORT FFTW3LibraryDepends
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})


# pkgconfig file
set (prefix ${CMAKE_INSTALL_PREFIX})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code, that doesn't depend on target, should be removed from function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm… It doesn't depend on the target, that's right, but those are the input variables for the following configure_file(…) call, so I think we should still keep them immediately before that call for readability. No damage done except for a couple CPU cycles.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those are the input variables

There is also code below:

export (TARGETS ${fftw3_lib} NAMESPACE FFTW3:: FILE ${PROJECT_BINARY_DIR}/FFTW3LibraryDepends.cmake)
install(EXPORT FFTW3LibraryDepends
        NAMESPACE FFTW3::
        DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/fftw3${PREC_SUFFIX}
        COMPONENT Development)

Need to test if it will work correctly if called several times.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That code below you quoted differs on each call of the function though. ${fftw3_lib} evaluates to fftw3, fftw3f ,fftw3l or fftw3q depending on the precision selected.

set (exec_prefix ${CMAKE_INSTALL_PREFIX})
set (libdir ${CMAKE_INSTALL_FULL_LIBDIR})
set (includedir ${CMAKE_INSTALL_FULL_INCLUDEDIR})
set (VERSION ${FFTW_VERSION})
configure_file (fftw.pc.in fftw${PREC_SUFFIX}.pc @ONLY)
install (FILES
${CMAKE_CURRENT_BINARY_DIR}/fftw${PREC_SUFFIX}.pc
DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig
COMPONENT Development)

# cmake file
set (FFTW3_LIBRARIES "FFTW3::${fftw3_lib}")
configure_file (FFTW3Config.cmake.in FFTW3${PREC_SUFFIX}Config.cmake @ONLY)
configure_file (FFTW3ConfigVersion.cmake.in FFTW3${PREC_SUFFIX}ConfigVersion.cmake @ONLY)
install (FILES
${CMAKE_CURRENT_BINARY_DIR}/FFTW3${PREC_SUFFIX}Config.cmake
${CMAKE_CURRENT_BINARY_DIR}/FFTW3${PREC_SUFFIX}ConfigVersion.cmake
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/fftw3${PREC_SUFFIX}
COMPONENT Development)

export (TARGETS ${fftw3_lib} NAMESPACE FFTW3:: FILE ${PROJECT_BINARY_DIR}/FFTW3LibraryDepends.cmake)
install(EXPORT FFTW3LibraryDepends
NAMESPACE FFTW3::
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/fftw3${PREC_SUFFIX}
COMPONENT Development)


if (BUILD_TESTS)

set (bench bench${PREC_SUFFIX})
add_executable (${bench} ${fftw_libbench2_SOURCE} tests/bench.c tests/hook.c tests/fftw-bench.c)

if (ENABLE_THREADS AND NOT WITH_COMBINED_THREADS)
target_link_libraries (${bench} ${fftw3_lib}_threads)
else ()
target_link_libraries (${bench} ${fftw3_lib})
endif ()

enable_testing ()

if (Threads_FOUND)

macro (fftw_add_test problem)
add_test (NAME ${problem} COMMAND ${bench} -s ${problem})
endmacro ()

fftw_add_test (32x64)
fftw_add_test (ib256)

endif ()

target_include_directories (${bench} PRIVATE "${CMAKE_CURRENT_BINARY_DIR}/${fftw3_lib}")
endif ()

endfunction ()

if (ENABLE_FLOAT)
add_fftw_library (SINGLE)
endif ()
if (ENABLE_DOUBLE)
add_fftw_library (DOUBLE)
endif ()
if (ENABLE_LONG_DOUBLE)
add_fftw_library (LDOUBLE)
endif ()
if (ENABLE_QUAD_PRECISION)
add_fftw_library (QUAD)
endif ()

foreach(subtarget ${subtargets})
set_target_properties (${subtarget} PROPERTIES SOVERSION 3.5.7 VERSION 3)
install (TARGETS ${subtarget}
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})
endforeach ()
install(TARGETS ${fftw3_lib}
EXPORT FFTW3LibraryDepends
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})

install (FILES api/fftw3.h DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
if (EXISTS api/fftw3.f)
install (FILES api/fftw3.f DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
endif ()

if (BUILD_TESTS)

add_executable (bench ${fftw_libbench2_SOURCE} tests/bench.c tests/hook.c tests/fftw-bench.c)

if (ENABLE_THREADS AND NOT WITH_COMBINED_THREADS)
target_link_libraries (bench ${fftw3_lib}_threads)
else ()
target_link_libraries (bench ${fftw3_lib})
endif ()


enable_testing ()

if (Threads_FOUND)

macro (fftw_add_test problem)
add_test (NAME ${problem} COMMAND bench -s ${problem})
endmacro ()

fftw_add_test (32x64)
fftw_add_test (ib256)

endif ()
endif ()

# pkgconfig file
set (prefix ${CMAKE_INSTALL_PREFIX})
set (exec_prefix ${CMAKE_INSTALL_PREFIX})
set (libdir ${CMAKE_INSTALL_FULL_LIBDIR})
set (includedir ${CMAKE_INSTALL_FULL_INCLUDEDIR})
set (VERSION ${FFTW_VERSION})
configure_file (fftw.pc.in fftw${PREC_SUFFIX}.pc @ONLY)
install (FILES
${CMAKE_CURRENT_BINARY_DIR}/fftw${PREC_SUFFIX}.pc
DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig
COMPONENT Development)

# cmake file
set (FFTW3_LIBRARIES "FFTW3::${fftw3_lib}")
configure_file (FFTW3Config.cmake.in FFTW3${PREC_SUFFIX}Config.cmake @ONLY)
configure_file (FFTW3ConfigVersion.cmake.in FFTW3${PREC_SUFFIX}ConfigVersion.cmake @ONLY)
install (FILES
${CMAKE_CURRENT_BINARY_DIR}/FFTW3${PREC_SUFFIX}Config.cmake
${CMAKE_CURRENT_BINARY_DIR}/FFTW3${PREC_SUFFIX}ConfigVersion.cmake
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/fftw3${PREC_SUFFIX}
COMPONENT Development)

export (TARGETS ${fftw3_lib} NAMESPACE FFTW3:: FILE ${PROJECT_BINARY_DIR}/FFTW3LibraryDepends.cmake)
install(EXPORT FFTW3LibraryDepends
NAMESPACE FFTW3::
DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/fftw3${PREC_SUFFIX}
COMPONENT Development)