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.

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

Allow setting FFTW_EXHAUSTIVE #2131

Merged
merged 6 commits into from
Nov 4, 2020
Merged

Allow setting FFTW_EXHAUSTIVE #2131

merged 6 commits into from
Nov 4, 2020

Conversation

johnomotani
Copy link
Contributor

@johnomotani johnomotani commented Nov 2, 2020

May sometimes make FFTs faster than the previously available FFTW_MEASURE setting.

Also uses a BOUT_ENUM_CLASS() for the option, which should help ensure that that doesn't get broken in future :-)

Unfortunately the macros don't work inside the bout::fft namespace, I think due to FFT_FLAG needing to be passed as a template parameter in various places. I don't understand why that didn't work though.

Performance - FFTs seemed to be faster with the exhaustive flag on Marconi, but I haven't checked with BOUT++ yet. Having the option for the exhaustive flag can't hurt anyway.

Todo:

  • document new option in manual

May sometimes make FFTs faster than the previously available
FFTW_MEASURE setting.
@johnomotani johnomotani added the backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 label Nov 2, 2020
src/invert/fft_fftw.cxx Outdated Show resolved Hide resolved
@johnomotani
Copy link
Contributor Author

The new option needs documenting in the manual. I'll do that before we merge.

@ZedThree
Copy link
Member

ZedThree commented Nov 2, 2020

Do we want to deprecate the fft_measure input? There's a v5 upgrader for renaming input parameters, it could probably also change the values at the same time.

The issue with putting a BOUT_ENUM_CLASS declaration in a namespace is that the specialisation of Options::as needs to be in the same namespace as Options. I managed to get it to work like this:

namespace bout {
enum class FFT_FLAG;
FFT_FLAG BOUT_MAKE_FROMSTRING_NAME(FFT_FLAG)(const std::string& s);
}

BOUT_ENUM_CLASS(bout::FFT_FLAG, estimate, measure);

but I can't see how to make it work in the macro without knowing the namespace name, i.e. BOUT_ENUM_CLASS(bout, FFT_FLAG, ...) can work.

ZedThree
ZedThree previously approved these changes Nov 2, 2020
src/invert/fft_fftw.cxx Outdated Show resolved Hide resolved
@ZedThree
Copy link
Member

ZedThree commented Nov 2, 2020

BTW, the reason I worked out why the namespace wasn't working was I tried compiling with clang, which gave me this error:

/home/peter/Codes/BOUT-dev/working/test_enum.cxx:8:1: error: function template specialization of 'as' not in class 'Options' or an enclosing namespace
BOUT_ENUM_CLASS(FFT_FLAG, estimate, measure);
^
/home/peter/Codes/BOUT-dev/include/bout/bout_enum_class.hxx:96:36: note: expanded from macro 'BOUT_ENUM_CLASS'
template <> inline auto ::Options::as<enumname>(const enumname&) const -> enumname { \
                                   ^
/home/peter/Codes/BOUT-dev/include/options.hxx:355:5: note: explicitly specialized declaration is here
  T as(const T& UNUSED(similar_to) = {}) const {
    ^
1 error generated.

whereas gcc gave me

In file included from /home/peter/Codes/BOUT-dev/working/test_enum.cxx:1:
/home/peter/Codes/BOUT-dev/working/test_enum.cxx:8:17: error: ‘FFT_FLAG’ does not name a type
    8 | BOUT_ENUM_CLASS(FFT_FLAG, estimate, measure);
      |                 ^~~~~~~~
/home/peter/Codes/BOUT-dev/working/test_enum.cxx:8:17: error: ‘FFT_FLAG’ does not name a type
    8 | BOUT_ENUM_CLASS(FFT_FLAG, estimate, measure);
      |                 ^~~~~~~~

Not quite as helpful as clang!

There are other FFTW flags, so make naming more specific in case we ever
want to use them.
Only keep fft_measurement_flag.
@ZedThree ZedThree merged commit 0e5e47a into next Nov 4, 2020
@ZedThree ZedThree deleted the fftw-exhaustive-next branch November 4, 2020 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants