Add number of OpenMP threads to dump file #2868

dschwoerer · 2024-02-20T08:45:38Z

Much nicer than having to parse the log to get the number of threads for e.g. plotting ...

github-actions · 2024-02-20T08:59:34Z

clang-tidy review says "All clean, LGTM! 👍"

dschwoerer · 2024-02-20T09:54:10Z

On 2/20/24 10:01, David Dickinson wrote: ***@***.**** commented on this pull request. It's a long time since I've worked with BOUT++ internals, but could the |ifdef| be replaced by a check of |options["use_openmp"]|?

I thought the omp_get_max_threads() function would only be definied if we use openmp, thus it might work with an if constexpr, but I am not sure whether that would be C++17? bout::build::use_openmp should be a constexpr, and thus usable in a if constexpr ...

d7919 · 2024-02-20T11:02:50Z

On 2/20/24 10:01, David Dickinson wrote: @.**** commented on this pull request. It's a long time since I've worked with BOUT++ internals, but could the |ifdef| be replaced by a check of |options["use_openmp"]|?
I thought the omp_get_max_threads() function would only be definied if we use openmp, thus it might work with an if constexpr, but I am not sure whether that would be C++17? bout::build::use_openmp should be a constexpr, and thus usable in a if constexpr ...

Yes sorry, immediately after posting the comment I realised it was a silly suggestion so deleted it.

github-actions · 2024-02-21T02:27:13Z

clang-tidy review says "All clean, LGTM! 👍"

dschwoerer · 2024-02-21T08:59:34Z

Sorry, I was confused why I couldn't see it on the web.

Looking at: https://en.cppreference.com/w/cpp/language/if it seems that would be possible if that was within a function, but does not seem to be possible in normal function.

We could add int omp_get_max_threads() {return 1}; to a header, for the case without openmp. That would avoid #ifdef in 2 or 3 places ...

ZedThree · 2024-02-21T09:24:47Z

We could add int omp_get_max_threads() {return 1}; to a header, for the case without openmp. That would avoid #ifdef in 2 or 3 places

This could be generally useful 👍

Does it make sense to actually return 0 when we don't have OpenMP?

dschwoerer · 2024-02-21T09:28:41Z

There is one case where it may be set to 0 without openmp, but in other cases it is 1.
I think also from a logical point of view, no openmp is mostly equivalent to 1 thread.

github-actions

clang-tidy made some suggestions

src/invert/laplace/impls/multigrid/multigrid_laplace.cxx

github-actions · 2024-02-21T10:03:58Z

clang-tidy review says "All clean, LGTM! 👍"

include/bout/openmpwrap.hxx

Co-authored-by: Peter Hill <[email protected]>

ZedThree

Lovely, thanks @dschwoerer ! Always nice to remove some #ifdefs

github-actions · 2024-02-21T10:15:28Z

clang-tidy review says "All clean, LGTM! 👍"

ZedThree · 2024-02-21T10:15:50Z

There is one case where it may be set to 0 without openmp, but in other cases it is 1. I think also from a logical point of view, no openmp is mostly equivalent to 1 thread.

I'm on the fence about this. In some sense, without OpenMP we have no threads -- there's definitely a measurable difference between 1 thread and no OpenMP, but I guess we do also store whether or not OpenMP is enabled, so we can use that in post-processing if necessary.

bendudson · 2024-02-21T15:47:45Z

I vote for 0 == no OpenMP ; 1 == OpenMP with one thread

include/bout/array.hxx

ZedThree · 2024-02-21T17:11:05Z

The OpenMP builds are failing because we're missing

#include "bout/build_defines.hxx"

in openmpwrap.hxx in order to bring in the BOUT_USE_OPENMP macro. I'm a bit worried this means the BOUT_OMP macro has been a no-op in several files for some time!

Otherwise the header does not expose the correct definitions and macros, leading to potential bugs.

github-actions · 2024-02-21T19:54:35Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-02-21T20:53:45Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions

clang-tidy made some suggestions

include/bout/array.hxx

github-actions · 2024-02-21T22:14:29Z

include/bout/openmpwrap.hxx

+inline int constexpr omp_get_num_threads() { return 1; }
+inline int constexpr omp_get_thread_num() { return 0; }
+#else
+#error OpenMP used but BOUT++ thinks it is disabled


warning: OpenMP used but BOUT++ thinks it is disabled [clang-diagnostic-error]

#error OpenMP used but BOUT++ thinks it is disabled ^

src/bout++.cxx

src/invert/laplace/impls/multigrid/multigrid_laplace.cxx

github-actions · 2024-02-21T22:20:49Z

clang-tidy review says "All clean, LGTM! 👍"

dschwoerer · 2024-02-22T08:32:32Z

The cuda builds are worrying me. They seem to have openmp enabled, but BOUT_USE_OPENMP is disabled. That means that all the thread-safety feature we can enable are not enabled. That might work most of the time (I have not tried) but can lead to race conditions, data corruption (two threads using the same data block) and crashes (random and annoying, but still better then simply wrong results).

I have added a check now, which is why the cuda build fails. One solution would be to just enable openmp for BOUT++ for cuda.
Is that a viable approach? Do we need to options, one for ensuring thread safety, and one for parallelising? Why is openmp enabled for cuda?

The other builds with openmp are failing in the test for bout-config - for some reason -fopenmp does not get added to it, I am confused as to why that fails ...

ZedThree · 2024-02-22T09:36:34Z

Verbose CUDA build here: https://github.com/boutproject/BOUT-dev/actions/runs/8002349081/job/21855360746

ZedThree · 2024-02-22T09:38:09Z

Also, I noticed has-openmp doesn't appear in the output of bout-config --all

dschwoerer · 2024-02-22T09:58:44Z

So something adds -fopenmp. @ggeorgakoudis did you add that intentionally? Is that needed? If so, I think we should just enable openmp for bout++ as well. Then we get the thread safety. If it turns out for performance reasons we do want to enable openmp but not have bout++ do parallelisation, we can add that option later. This PR is already enormous for adding 1 single integer to the dump file ...

ZedThree · 2024-02-22T10:06:29Z

It might be coming from RAJA or UMPIRE, it doesn't appear to be turned on after we detect CUDA: https://github.com/boutproject/BOUT-dev/actions/runs/8002552988/job/21856012784#step:4:64

OpenMP is enabled any way, but if bout is not aware of using openmp, it might misbehave.

github-actions · 2024-02-22T10:28:23Z

clang-tidy review says "All clean, LGTM! 👍"

BOUT_OMP is split in BOUT_OMP_SAFE for cases where we want to ensure that bout++ behaves correctly in an openmp parallel environement. BOUT_OMP_PERF on the other hand enables using openmp for parallel regions. BOUT_OMP_SAFE is enabled whenever openmp is detected, while BOUT_OMP_PERF is a user option.

github-actions · 2024-02-22T12:05:41Z

clang-tidy review says "All clean, LGTM! 👍"

ZedThree · 2024-02-22T14:18:28Z

Thanks for fixing this @dschwoerer, but it feels a bit like overkill now, as you say just to add one flag to the output!

It sort of sidesteps the issue of why the CUDA build is getting OpenMP turned on when we haven't requested it. @ggeorgakoudis Is this coming from the RAJA dependency? Should we actually be forcing OpenMP on in BOUT++ if RAJA is built with OpenMP?

Just because a dependency uses OpenMP doesn't necessarily mean we need to enable it in BOUT++ to ensure thread safety -- that depends on whether we're passing callbacks that include thread unsafe code.

ggeorgakoudis · 2024-02-22T15:35:22Z

@ZedThree No, I haven't made any changes to the build system so I do not explicitly set openmp flags in compilation. It may be picked up by including RAJA (since its spack installation in the CI container includes the openmp variant), although I haven't found the exact spot in the cmake file hierarchy where this happens (let me know if you spot it). We can enable openmp in the CUDA CI configuration (https://github.com/boutproject/BOUT-dev/actions/runs/8002552988/job/21856012784#step:4:15) to avoid BOUT++ thinking it builds without openmp when RAJA pulls that in. Makes sense?

dschwoerer · 2024-02-22T19:25:03Z

Thanks for fixing this @dschwoerer, but it feels a bit like overkill now, as you say just to add one flag to the output!

I did not mean overkill, I just meant feature creep. I think there was a bug that hasn't been noticed before and this fixes it.
There was some consideration for parallelising Hermes-3 with OpenMP, and for this we would also need to be thread safe, but not parallelise the for loops. Thus this PR might be interesting for @bendudson as well ...

Just because a dependency uses OpenMP doesn't necessarily mean we need to enable it in BOUT++ to ensure thread safety -- that depends on whether we're passing callbacks that include thread unsafe code.

Why would this be limited to callbacks?
If the function calls with different threats into some function that allocations some memory, and we use our cached malloc pool, that could lead to issues without any callbacks.

I think BOUT++ should always try to be on the safe side. Otherwise we could go back to commit 62f3d2c and ignore all of the other changes ... however I think it is not so bad, compared to getting sometimes wrong results because some memory block is double used and contains the wrong data.

dschwoerer and others added 2 commits February 20, 2024 09:43

Add number of OpenMP threads to dump file

62f3d2c

Apply clang-format changes

dc89b52

dschwoerer and others added 2 commits February 21, 2024 03:12

Ignore number of openmp_threads in test

2d289cc

Apply black changes

2d9858a

dschwoerer and others added 2 commits February 21, 2024 10:54

Avoid some #if branching for openmp

694d8c0

Apply clang-format changes

436b6cd

github-actions bot reviewed Feb 21, 2024

View reviewed changes

src/invert/laplace/impls/multigrid/multigrid_laplace.cxx Show resolved Hide resolved

Add additional openmp shim

fe91ff0

ZedThree reviewed Feb 21, 2024

View reviewed changes

include/bout/openmpwrap.hxx Outdated Show resolved Hide resolved

function definitions in headers should be inline

3d9a53d

Co-authored-by: Peter Hill <[email protected]>

ZedThree previously approved these changes Feb 21, 2024

View reviewed changes

dschwoerer commented Feb 21, 2024

View reviewed changes

include/bout/array.hxx Show resolved Hide resolved

Include build_defines

3e737b0

Otherwise the header does not expose the correct definitions and macros, leading to potential bugs.

dschwoerer dismissed ZedThree’s stale review via 3e737b0 February 21, 2024 19:46

Include openmp header

adbe88a

Add -fopenmp to FLAGS if using openmp

828552b

Ensure that _OPENMP and BOUT_USE_OPENMP is the same

f9b8426

github-actions bot reviewed Feb 21, 2024

View reviewed changes

Fix ifdef

57bb653

dschwoerer added 3 commits February 22, 2024 11:22

Track ldflags for shared lib

82643e2

Print --has-openmp in --all for bout-config

30cb546

CI: Enable openmp for cuda

748998f

OpenMP is enabled any way, but if bout is not aware of using openmp, it might misbehave.

dschwoerer and others added 3 commits February 22, 2024 12:33

Apply clang-format changes

ecef0e6

Ensure omp functions are always defined

2a1185d

bendudson approved these changes Feb 23, 2024

View reviewed changes

bendudson merged commit 5d0a1ad into next Feb 23, 2024
27 of 28 checks passed

bendudson deleted the dump-openmp branch February 23, 2024 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add number of OpenMP threads to dump file #2868

Add number of OpenMP threads to dump file #2868

dschwoerer commented Feb 20, 2024

github-actions bot commented Feb 20, 2024

dschwoerer commented Feb 20, 2024 via email

d7919 commented Feb 20, 2024

github-actions bot commented Feb 21, 2024

dschwoerer commented Feb 21, 2024

ZedThree commented Feb 21, 2024

dschwoerer commented Feb 21, 2024

github-actions bot left a comment

github-actions bot commented Feb 21, 2024

ZedThree left a comment •

edited

Loading

github-actions bot commented Feb 21, 2024

ZedThree commented Feb 21, 2024

bendudson commented Feb 21, 2024

ZedThree commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot left a comment

github-actions bot Feb 21, 2024

github-actions bot commented Feb 21, 2024

dschwoerer commented Feb 22, 2024

ZedThree commented Feb 22, 2024

ZedThree commented Feb 22, 2024

dschwoerer commented Feb 22, 2024

ZedThree commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

ZedThree commented Feb 22, 2024

ggeorgakoudis commented Feb 22, 2024

dschwoerer commented Feb 22, 2024

Add number of OpenMP threads to dump file #2868

Add number of OpenMP threads to dump file #2868

Conversation

dschwoerer commented Feb 20, 2024

github-actions bot commented Feb 20, 2024

dschwoerer commented Feb 20, 2024 via email

d7919 commented Feb 20, 2024

github-actions bot commented Feb 21, 2024

dschwoerer commented Feb 21, 2024

ZedThree commented Feb 21, 2024

dschwoerer commented Feb 21, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2024

ZedThree left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2024

ZedThree commented Feb 21, 2024

bendudson commented Feb 21, 2024

ZedThree commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot commented Feb 21, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Feb 21, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2024

dschwoerer commented Feb 22, 2024

ZedThree commented Feb 22, 2024

ZedThree commented Feb 22, 2024

dschwoerer commented Feb 22, 2024

ZedThree commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

ZedThree commented Feb 22, 2024

ggeorgakoudis commented Feb 22, 2024

dschwoerer commented Feb 22, 2024

ZedThree left a comment •

edited

Loading