De-macroize FFT

[mreineck]

Next step in the de-macroizing effort.

[DiamonDinoia]

I think is good. Only two suggestions.

1. Could you coordinate with @blackwer for PR Provide a user level locking mechanism for FFTW #548 ? So that we do not do the same work twice?
2. Can we have a generic fftPlan that with ifdefs for ducc or fftw inside fft.cpp? Since ducc0 plan does nothing fft.plan does nothing for ducc and does the planning for fftw.

[mreineck]

It feels like #548 will cause huge collisions with this PR, so I suggest that #548 goes in first and then I see how I can salvage things.
If you want an empty plan for ducc FFT, I can certainly add that.

[blackwer]

It feels like #548 will cause huge collisions with this PR, so I suggest that #548 goes in first and then I see how I can salvage things. If you want an empty plan for ducc FFT, I can certainly add that.

FWIW, I probably won't finish that PR until mid next week probably. There's more investigation about the cause of the locking bug that I need to complete, and have other things on my plate right now

[mreineck]

I'm not in a hurry myself, so it's absolutely fine from my side to wait.

[DiamonDinoia]

It feels like #548 will cause huge collisions with this PR, so I suggest that #548 goes in first and then I see how I can salvage things. If you want an empty plan for ducc FFT, I can certainly add that.

I think having a plan in finufft.cpp and all the ifdefs in fft.cpp is better for readability. So that if someone wants to understand the math in finufft.cpp they can do so without worrying on who and how the fft is done. There's just and fft there.

[mreineck]

Most of the conditional compilation is now gone. The remaining conditional section is where constants from FFTW are used and where the actual execute() call is made. There will now be a useless printout when using ducc, informing the user that plan preparation took 0 seconds :-)

[DiamonDinoia]

#548 got merged. Can you update this? Can you also silence the warning about datatypes that appear on github? I was responsible or the mess on the other files but I will try to clean them as they make code reviews tedious.

[mreineck]

I'll try to update the patch tomorrow. If the custom locking function is supposed to supersede our own locking completely, this might get interesting ...
Concerning the warnings: I can probably silence most of them, but instead of making the code safer, it will only make it uglier; do we want that?
(Do we really want an automatic functionality that gives us a wall of text whenever we define somehing inline in a header that is unused in the current translation unit? I guess we can be happy that it doesn't warn us about unused macros :-) )

[mreineck]

The way the custom locking mechanism is currently implemented does not work with plan-independent calls like FFTW_FORGET_WISDOM and FFTW_CLEANUP. Do we want to lock these as well? I'm asking because that's what I'm doing at the moment on this branch, but I can't use the custom locking mechanism for it, since I don't have access to p->opts in those circumstances.

[blackwer]

The way the custom locking mechanism is currently implemented does not work with plan-independent calls like FFTW_FORGET_WISDOM and FFTW_CLEANUP. Do we want to lock these as well? I'm asking because that's what I'm doing at the moment on this branch, but I can't use the custom locking mechanism for it, since I don't have access to p->opts in those circumstances.

Since those are internal helper functions for testing and not part of the implementation or the API, I don't think we should mess with locking them at all. Is there a use case to bother?

[mreineck]

I'm happy to remove locking from these entirely!

[DiamonDinoia]

I'll try to update the patch tomorrow. If the custom locking function is supposed to supersede our own locking completely, this might get interesting ... Concerning the warnings: I can probably silence most of them, but instead of making the code safer, it will only make it uglier; do we want that? (Do we really want an automatic functionality that gives us a wall of text whenever we define somehing inline in a header that is unused in the current translation unit? I guess we can be happy that it doesn't warn us about unused macros :-) )

I leave the warning to your judgement :)

[mreineck]

I've added a few experiments to work around the warnings ... let's see.

[mreineck]

The warnings about padding inside FINUFFT_PLAN_S can be fixed by changing the order or member variables. Should I do that?

[mreineck]

OK, that's all the warning fixes I could make (without restructuring the plan type).

[DiamonDinoia]

Just minor changes. Then we can merge.

[DiamonDinoia]

Just minor changes. Then we can merge.

[DiamonDinoia]

The warnings about padding inside FINUFFT_PLAN_S can be fixed by changing the order or member variables. Should I do that?

Yes, that is a good idea. I might disable that warning entirely at some point since these are not crucial. Unlikely to create an array with thousands of plans.

[mreineck]

I'm not sure I understand. Should I rearrange struct members, or is it better to disable the warning at some point?
Rearranging would mean that members that "belong together", like sortIndices and didSort, will move far apart in the class body.

[DiamonDinoia]

I'm not sure I understand. Should I rearrange struct members, or is it better to disable the warning at some point? Rearranging would mean that members that "belong together", like sortIndices and didSort, will move far apart in the class body.

I see, I did not think of that. Let's disable the warning in the future. Readability is better in this case

[mreineck]

OK, should be good now.

[DiamonDinoia]

Merging once pipeline finishes. For the future, using const and auto where possible is preferable. Is something, I will like to refactor at some point.

[mreineck]

Thanks, good to know!