De-macroize step 1/n: Probably uncontroversial switches from macros to constexpr

[mreineck]

I converted macros to constexpr definitions or static inline functions wherever I considered this safe. While at it, I also removed a few macros that were unused.

[DiamonDinoia]

@mreineck is it ready for review?

[mreineck]

Yes. From my point of view the open questions are

• does doc generation work with enums?
• what to do about the #if 1 / #else / #endif block in defs.h, which I left as a kind of reminder. We can certainly remove the #else part if that is considered nicer.

[DiamonDinoia]

Hi Martin, I am delaying this until 2.3 is out. We need to merge the GPU stuff and change cmake/docs. You can continue on this branching from this branch as I will get to merge this. I am delaying some changes to the CPU code for after the templating in order to not overlap.

[mreineck]

I think I'll add the subsequent steps after this one is merged. It's probably easier to do this at the right time than chase potential merge conflicts if I continue now.

[DiamonDinoia]

I checked the doc issue. Seems okay. If it passes CI we can merge.

The #ifdef in defs.h is okay for now otherwise things would break.

It is also nice that we can offer a nice c++ interface to the user.

[DiamonDinoia]

The new tests found an issue it seems. Let me know if you need help investigating. It might be something related to the optimization flags.

[mreineck]

As discussed yesterday, I merged master into the branch; hopefully this will fix the remaining test failures.

[mreineck]

OK, that doesn't seem to help...

[mreineck]

Problem seems to occur only for debug builds with static linking turned off.

I can reproduce it locally, and the error is huge. Something is really fishy, but the sanitizers don't seem to catch it ...

[mreineck]

If I read it right, this happens with both ducc FFT and FFTW.

[mreineck]

Maybe related, maybe not: if I compile with static linking disabled, I get the following error from cmake:

[ 71%] Linking C executable guru1d1c
cd /home/martin/codes/finufft/build/examples && /usr/bin/cmake -E cmake_link_script CMakeFiles/guru1d1c.dir/link.txt --verbose=1
/usr/bin/cc -g CMakeFiles/guru1d1c.dir/guru1d1c.c.o -o guru1d1c  -Wl,-rpath,/home/martin/codes/finufft/build ../libfinufft.so
/usr/bin/ld: CMakeFiles/guru1d1c.dir/guru1d1c.c.o: undefined reference to symbol 'cexp@@GLIBC_2.2.5'
/usr/bin/ld: /lib/x86_64-linux-gnu/libm.so.6: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

cexp is in libm, and I think when using the C compiler as linker, this has to be specified explicitly.

[mreineck]

I think I found it ... it's pretty nasty :-(

In principle, finufft is currently set up to be compiled as two different libraries, one for single precision, one for double precision. But cmake puts both single- and double-precision object files into a single .a or .so in the end. That means that constants like IMA and PI, which are different for both precisions end up twice in the same library, with the same name, and only the first occurrence will be used... sometimes it will be the wrong one.

I can fix this by reverting the numerical constants to macros until everything is fully templatized. Not great, but I can't think of anything better at the moment.

[mreineck]

OK, I think this is ready to merge now.

[DiamonDinoia]

I think I found it ... it's pretty nasty :-(

In principle, finufft is currently set up to be compiled as two different libraries, one for single precision, one for double precision. But cmake puts both single- and double-precision object files into a single .a or .so in the end. That means that constants like IMA and PI, which are different for both precisions end up twice in the same library, with the same name, and only the first occurrence will be used... sometimes it will be the wrong one.

I can fix this by reverting the numerical constants to macros until everything is fully templatized. Not great, but I can't think of anything better at the moment.

The alternative is to define the M_PI as constexpr functions:

template<typename T>
FINUFFT_ALWAYS_INLINE constexpr T PI() noexcept {
return static_cast<T>(value here);
}

[DiamonDinoia]

Maybe related, maybe not: if I compile with static linking disabled, I get the following error from cmake:

[ 71%] Linking C executable guru1d1c
cd /home/martin/codes/finufft/build/examples && /usr/bin/cmake -E cmake_link_script CMakeFiles/guru1d1c.dir/link.txt --verbose=1
/usr/bin/cc -g CMakeFiles/guru1d1c.dir/guru1d1c.c.o -o guru1d1c  -Wl,-rpath,/home/martin/codes/finufft/build ../libfinufft.so
/usr/bin/ld: CMakeFiles/guru1d1c.dir/guru1d1c.c.o: undefined reference to symbol 'cexp@@GLIBC_2.2.5'
/usr/bin/ld: /lib/x86_64-linux-gnu/libm.so.6: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

cexp is in libm, and I think when using the C compiler as linker, this has to be specified explicitly.

Yes, that is a issue I noticed a while ago and fixed in my current branch.
I need to link m to cuda examples on linux.

[ahbarnett]

Thanks for working on this. None of the constants are unusual ones, so maybe they are better found in math.h or whatever the C++ equiv is. Since I don't know templating well at all, I don't know the way to have T(pi) or T(complex(0,1)) best coded so there is no collision in library... but that's what should happen. Best, Alex

…

On Wed, Aug 14, 2024 at 10:24 AM Marco Barbone ***@***.***> wrote: Maybe related, maybe not: if I compile with static linking disabled, I get the following error from cmake: [ 71%] Linking C executable guru1d1c cd /home/martin/codes/finufft/build/examples && /usr/bin/cmake -E cmake_link_script CMakeFiles/guru1d1c.dir/link.txt --verbose=1 /usr/bin/cc -g CMakeFiles/guru1d1c.dir/guru1d1c.c.o -o guru1d1c -Wl,-rpath,/home/martin/codes/finufft/build ../libfinufft.so /usr/bin/ld: CMakeFiles/guru1d1c.dir/guru1d1c.c.o: undefined reference to symbol 'cexp@@GLIBC_2.2.5' /usr/bin/ld: /lib/x86_64-linux-gnu/libm.so.6: error adding symbols: DSO missing from command line collect2: error: ld returned 1 exit status cexp is in libm, and I think when using the C compiler as linker, this has to be specified explicitly. Yes, that is a issue I noticed a while ago and fixed in my current branch. I need to link m to cuda examples on linux. — Reply to this email directly, view it on GitHub <#482 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACNZRSSG4T2CODOBTGW4XPTZRNR53AVCNFSM6AAAAABK6HYAYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBYHEZDGOJZGM> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- *-------------------------------------------------------------------~^`^~._.~' |\ Alex Barnett Center for Computational Mathematics, Flatiron Institute | \ http://users.flatironinstitute.org/~ahb 646-876-5942

[mreineck]

The "proper" solution for this is templated constants, which only arrived in C++20 unfortunately...

But in any case, once there are no more files that are compiled twice (except for the few lines of the standard C interface), all of this will no longer be an issue. So I propose to go with the macros for now; they will vanish in one of the subsequent pull requests.

[DiamonDinoia]

It looks good. Merging after 2.3.0 is out.

[DiamonDinoia]

Hi @mreineck, we are finally ready to merge! Can you resolve the conflicts?

[mreineck]

Done (I think)!