CMake: Adding support for building multiple precisions at once by emmenlau · Pull Request #276 · FFTW/fftw3

emmenlau · 2022-03-09T21:33:23Z

This PR allows to build multiple (or all) precisions in a single go in cmake. Please let me know what you think?

emmenlau · 2022-04-02T13:08:04Z

The PR does not fully work out of the box, because it will not select the sources dynamically for the precision. I can provide an improved version if this is relevant at all?

LecrisUT

Most of the comments are regarding the cmake project in general. Maybe these can be spun up in a different PR. Also what do you mean selecting the sources dynamically for the precision? Also, mark this as Depends on #275?

LecrisUT · 2022-12-31T16:21:12Z

CMakeLists.txt


+if (ENABLE_NEON)
+  if (ENABLE_LONG_DOUBLE)
+    message (FATAL_ERROR "NEON only works in single or double precision, please disable long double support")


This seems contradictory to this PR. Shouldn't it be fine that NEON is only built for single and double, and we just give a warning that it doesn't work for the others?

LecrisUT · 2022-12-31T16:22:01Z

CMakeLists.txt

-  set (BENCHFFT_LDOUBLE TRUE)
-  set (PREC_SUFFIX l)
-endif ()
+set (FFTW_VERSION 3.3.10)


This is rather archaic. We should move this into project(FFTW VERSION 3.3.10)

Also in the project, only C language should be enabled

LecrisUT · 2022-12-31T16:26:33Z

CMakeLists.txt

-  set (PREC_SUFFIX q)
-endif ()
-set (fftw3_lib fftw3${PREC_SUFFIX})
+set (PRECISIONS SINGLE DOUBLE LDOUBLE QUAD)


How about passing this as a list via set(CACHE), then if any flags are added, simply add to the list.

LecrisUT · 2022-12-31T16:33:40Z

CMakeLists.txt

+  configure_file (cmake.config.h.in "${PRECISION}/config.h" @ONLY)
+  target_include_directories (${fftw3_lib} PRIVATE "${CMAKE_CURRENT_BINARY_DIR}/${PRECISION}")
+
+  target_include_directories (${fftw3_lib} INTERFACE $<INSTALL_INTERFACE:include>)


$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>

~~Oops, I understood this tutorial the other way around~~.

This is still recommended. Just don't use ${CMAKE_INSTALL_FULL_INCLUDEDIR}

LecrisUT · 2022-12-31T16:34:50Z

CMakeLists.txt

+  target_include_directories (${fftw3_lib} INTERFACE $<INSTALL_INTERFACE:include>)
+
+  if (BUILD_SHARED_LIBS)
+    target_compile_definitions (${fftw3_lib} PRIVATE -DFFTW_DLL)


Is this specific to windows?

LecrisUT · 2022-12-31T16:41:31Z

CMakeLists.txt

+    else ()
+      add_library (${fftw3_lib}_threads ${fftw_threads_SOURCE})
+      target_include_directories (${fftw3_lib}_threads PRIVATE "${CMAKE_CURRENT_BINARY_DIR}/${PRECISION}")
+      target_include_directories (${fftw3_lib}_threads INTERFACE $<INSTALL_INTERFACE:include>)


Seems these can be generalized in an interface target as well. Can combine with depends to link to appropriate config.h as well.

LecrisUT · 2022-12-31T16:42:35Z

CMakeLists.txt

+  endif ()
+
+  foreach(subtarget ${subtargets})
+    set_target_properties (${subtarget} PROPERTIES SOVERSION 3.6.9 VERSION 3)


Why are SOVERSIONs not in sync with PROJECT_VERSION?

LecrisUT · 2022-12-31T16:46:57Z

CMakeLists.txt

+  foreach(subtarget ${subtargets})
+    set_target_properties (${subtarget} PROPERTIES SOVERSION 3.6.9 VERSION 3)
+    install (TARGETS ${subtarget}
+        RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}


These should be exported as well

LecrisUT · 2022-12-31T16:52:52Z

CMakeLists.txt

+
+    enable_testing ()
+
+    if (Threads_FOUND)


Either move this together with condition BUILD_TESTS, or enable the testing altogether. Also bench is not a good name for this purpose. Preferably move these to tests subfolder

LecrisUT · 2022-12-31T16:54:57Z

CMakeLists.txt

-          ${CMAKE_CURRENT_BINARY_DIR}/FFTW3${PREC_SUFFIX}ConfigVersion.cmake
-	  DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/fftw3${PREC_SUFFIX}
-         COMPONENT Development)
+    ${CMAKE_CURRENT_BINARY_DIR}/fftw3${PREC_SUFFIX}.pc


These should be in the loop

emmenlau · 2023-01-03T08:47:16Z

Hi @LecrisUT , thanks for your interest to push the cmake support forward, it's appreciated. However I should note that currently the commit CMakeLists.txt: Support multi-precision builds
is broken because it makes some wrong assumption about which code needs to be built. I don't even recall the details but there was a flaw that requires slightly larger re-design of the change.

LecrisUT · 2023-01-03T10:22:00Z

The only part that I can see is the lack of target_compile_definitions for FFTW_SINGLE etc. Check these lines:

fftw3/kernel/ifftw.h

Lines 63 to 78 in 9426cd5

    
           #define CONCAT(prefix, name) prefix ## name 
        
           #if defined(FFTW_SINGLE) 
        
             typedef float R; 
        
           # define X(name) CONCAT(fftwf_, name) 
        
           #elif defined(FFTW_LDOUBLE) 
        
             typedef long double R; 
        
           # define X(name) CONCAT(fftwl_, name) 
        
           # define TRIGREAL_IS_LONG_DOUBLE 
        
           #elif defined(FFTW_QUAD) 
        
             typedef __float128 R; 
        
           # define X(name) CONCAT(fftwq_, name) 
        
           # define TRIGREAL_IS_QUAD 
        
           #else 
        
             typedef double R; 
        
           # define X(name) CONCAT(fftw_, name) 
        
           #endif

emmenlau · 2023-06-25T19:44:59Z

Hi @LecrisUT , thanks for your interest to push the cmake support forward, it's appreciated. However I should note that currently the commit CMakeLists.txt: Support multi-precision builds is broken because it makes some wrong assumption about which code needs to be built. I don't even recall the details but there was a flaw that requires slightly larger re-design of the change.

Hey @LecrisUT , I've re-considered this PR and I'm not sure whether there is a problem with my changes or not. It may help if you could try the fftw built with these instructions here and see if it is any faster or slower than the "common" fftw. Is that something you can test?

LecrisUT · 2023-06-25T21:03:07Z

Honestly I have completely lost interest in contributing to modernizing this project after the comments in #307. I have no hope that any improvements we make will be considered. Cmake fully supports OCaml and the project can be made to be fully compiled with cmake, but until that is being considered I don't think I'll participate.

JiahanBro · 2023-06-26T08:17:03Z

________________________________ Von: Cristian Le ***@***.***> Gesendet: Sonntag, 25. Juni 2023 23:03 An: FFTW/fftw3 ***@***.***> Cc: Subscribed ***@***.***> Betreff: Re: [FFTW/fftw3] CMake: Adding support for building multiple precisions at once (PR #276) Honestly I have completely lost interest in contributing to modernizing this project after the comments in #307<#307>. I have no hope that any improvements we make will be considered. Cmake fully supports OCaml and the project can be made to be fully compiled with cmake, but until that is being considered I don't think I'll participate. — Reply to this email directly, view it on GitHub<#276 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGRKXG5PKUEJZGATTK3P2WTXNCRRZANCNFSM5QKYXOPA>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

LecrisUT · 2023-06-26T08:37:35Z

@JiahanBro, did you meant to forward that message internally or make a comment?

emmenlau added 3 commits March 9, 2022 22:32

CMakeLists.txt: Updated version to 3.3.10

31e3451

CMake: Added ARM Neon support

632233e

CMakeLists.txt: Support multi-precision builds

a2a0a78

emmenlau force-pushed the bda_multi_prec_build branch from 75b64eb to a2a0a78 Compare March 10, 2022 20:40

LecrisUT suggested changes Dec 31, 2022

View reviewed changes

LecrisUT mentioned this pull request Jan 2, 2023

Clarify current/future cmake support #307

Open

Conversation

emmenlau commented Mar 9, 2022

Uh oh!

emmenlau commented Apr 2, 2022

Uh oh!

LecrisUT left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LecrisUT Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emmenlau commented Jan 3, 2023

Uh oh!

LecrisUT commented Jan 3, 2023

Uh oh!

emmenlau commented Jun 25, 2023

Uh oh!

LecrisUT commented Jun 25, 2023

Uh oh!

JiahanBro commented Jun 26, 2023 via email

Uh oh!

LecrisUT commented Jun 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LecrisUT left a comment •

edited

Loading

LecrisUT Jan 2, 2023 •

edited

Loading