Skip to content

Guard omp_lib so serial and non-GNU builds compile - #514

Open
krystophny wants to merge 1 commit into
mainfrom
fix/serial-build-without-openmp
Open

Guard omp_lib so serial and non-GNU builds compile#514
krystophny wants to merge 1 commit into
mainfrom
fix/serial-build-without-openmp

Conversation

@krystophny

Copy link
Copy Markdown
Member

Problem

Six files referenced omp_lib without the !$ sentinel. That is a hard semantic error, not a warning, whenever the module is absent:

  • -DENABLE_OPENMP=OFF on any compiler
  • every flang build on a toolchain whose OpenMP runtime ships no omp_lib.mod (LLVM 22 here does not; LLVM 23 does)
error: Cannot parse module file for module 'omp_lib': Source file 'omp_lib.mod' was not found

Fix

Each site gets the sentinel plus a serial fallback:

  • simple_main takes wall time from timing's get_wtime(), which already documents itself as "replacement for omp_get_wtime", and routes the two debug prints through a guarded local thread id.
  • simple_gpu defaults its device index to 0 — the same value the single-device path already implies when ngpu <= 1.
  • classification, test_coord_trans import no symbols, so the sentinel alone suffices.

Test

test/python/test_openmp_optional.py rejects any unguarded reference. Its oracle is the Fortran rule that such a line cannot compile without OpenMP, so it constrains files that do not exist yet rather than pinning current contents. It reports 13 findings on the parent commit and passes on this one.

Verification

build result
gfortran + OpenMP (Release) unchanged, builds
flang 23 + OpenMP (Release) all 1209 targets
flang 22, ENABLE_OPENMP=OFF builds

gfortran and flang-23 agree bit-for-bit on a Boozer-field case (midpoint integrator, 8 threads, pinned start.dat): 0.0e+00 max relative difference on loss times, final coordinates and confined fraction, at trace times 6e-5, 1e-3 and 1e-2 s.

Incidental finding, not addressed here: sample_particles_test_field ignores startmode and calls random_number, so isw_field_type = -1 cases cannot be compared across compilers — gfortran and flang have different PRNGs. Any cross-compiler check needs a real field with startmode = 2.

@krystophny krystophny added tier/T3 physics or output behavior size/S review size up to 100 changed lines labels Aug 5, 2026
@krystophny

Copy link
Copy Markdown
Member Author

Test suite state, clean build tree (CMAKE_BUILD_TYPE=Release, defaults):

109/116 pass. All 7 failures reproduce on the parent commit b44de79 in an identical configuration, so none are introduced here:

test cause
test_e2e_boozer_chartmap ModuleNotFoundError: netCDF4
test_spectre_sympl_volume ModuleNotFoundError: netCDF4
test_spectre_sympl_crossing ModuleNotFoundError: netCDF4
orbit_netcdf_verify, orbit_netcdf_plot fails on parent too
golden_record_albert_coils, golden_record_canonical fails on parent too

One worth flagging separately: test_spectre_validation fails with mu: GC-map mu scatter 3.606e-02 >= 5e-03 in a stale build tree, and passes in a clean one. It fails on the parent commit in that same stale tree, so it is configuration-sensitive, not commit-sensitive. The stale tree differed by SIMPLE_DETERMINISTIC_FP=ON and a libneo pinned at 2e34486 against bc36357 fresh. Might be worth someone checking whether that assertion is genuinely sensitive to the libneo version.

@slopqueue slopqueue Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review verdict: Approve

Summary: This PR (4 commits) makes the classifier's continuous decision inputs available for downstream confinement scoring and fixes two real defects. (1) omp_lib references are guarded with the !$ sentinel so serial/non-GNU builds compile (verified by a new test_openmp_optional.py lint that I ran locally — clean across all 173 sources). (2) fast_class alone now enables classification dispatch via a new classification_enabled() predicate in params.f90, fixing the bug where fast_class=True with tcut<=0, class_plot=.F. silently traced ordinary orbits. (3) class_scores.dat exposes jpar_spread, jpar_ref, topology_margin, a score_status flag, and the new tip radial_spread/tip_count, kept in a separate file so class_parts.dat's column contract is preserved. I verified the new predicate against all 10 rows of bminmax_cache_cases.tsv (Fortran and Python both match), confirmed the refactored check_orbit_type monotonicity margin produces byte-identical integer classes (the removed exits were only an early-exit optimization; margin<0 ⟺ ideal=2 holds), and confirmed get_wtime() type-compatibly replaces omp_get_wtime(). Coverage is good (new driver tests + extended lifecycle cases). I could not run the full make test because the environment lacks BLAS/LAPACK.

Findings:

  1. [minor] python/pysimple/init.py:472 — the only real caller of _needs_bminmax_cache still passes only (params.num_surf, params.ntcut, params.class_plot) and omits params.fast_class, even though this PR added the fast_class parameter precisely to keep Python in sync with the Fortran classification_enabled() predicate. It is currently harmless only because the num_surf != 1 fallback coincidentally yields the same result as the Fortran num_surf > 1 branch for every num_surf, but it contradicts the PR's own "three call sites must agree" docstring and silently diverges the moment either predicate is touched. Fix: pass params.fast_class (or read it from params inside the function) so the Python cache gate tracks the Fortran dispatch.

  2. [minor] test/tests/test_class_scores_driver.py:31 — the oracle re-thresholds jpar_spread read back from the ASCII class_scores.dat against a hardcoded TOL_PERPINV = 15.0, duplicating tol_perpinv = 15.d0 in src/check_orbit_type.f90 (and the underlying drift > tol_perpinv test). Both use strict >, so borderline values near exactly 15.0 could round across the threshold on a different field/compiler and flake. It passes on the fixed deterministic test field, but deriving the constant from the Fortran source (or testing a threshold-aware interval) would be more robust.

Verdict: Approve — the changes are correct, well-tested, and both stated defects (serial-build breakage and fast_class silently doing nothing) are genuinely fixed; the two findings above are non-blocking robustness/consistency notes.

Six files referenced omp_lib without the `!$` sentinel, so the build broke
outright whenever the module was absent: ENABLE_OPENMP=OFF on any compiler,
and every flang build on a toolchain whose OpenMP runtime ships no
omp_lib.mod. The failure was a hard semantic error, not a warning.

Each site now has a serial fallback. simple_main uses timing's get_wtime(),
which already documents itself as the omp_get_wtime replacement, and the two
debug prints take the thread id through a guarded local. simple_gpu defaults
its device index to 0, matching the single-device path it already takes when
ngpu <= 1.

test_openmp_optional.py rejects any unguarded reference. Its oracle is the
Fortran rule that such a line cannot compile without OpenMP, so it constrains
files that do not exist yet; it reports 13 findings on the parent commit.

Verified: gfortran with OpenMP unchanged, flang 23 with OpenMP builds all
1209 targets, and the two agree bit-for-bit (0.0e+00 on loss times, final
coordinates and confined fraction) on a Boozer-field case at 8 threads with
pinned starting conditions.
@krystophny
krystophny force-pushed the fix/serial-build-without-openmp branch from 88a1f5b to 8bd460d Compare August 10, 2026 09:34
@krystophny

Copy link
Copy Markdown
Member Author

Verified locally, and it surfaced a dependency.

Serial build with -DENABLE_OPENMP=OFF -DCMAKE_BUILD_TYPE=Release:

So this PR needs libneo#411 to demonstrate its own fix on a host without f2py. The OpenMP guard itself is sound — with #411 in place the serial build completes end to end.

Separately: this branch was rebased onto main. It previously carried 864d817 (all of #512) and 1d27012 + b44de79 (part of #513) while declaring base: main, so GitHub showed a cumulative +481/-46 across 15 files. It is now the single guard commit, 7 files, +101/-13. The remaining non-OpenMP lines are the get_wtime() wrapper replacing omp_get_wtime(), part of the same change.

@slopqueue slopqueue Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review verdict: Request changes

Summary: This PR makes the SIMPLE build independent of OpenMP by guarding every use omp_lib and omp_get_* reference with the standard !$ conditional-compilation sentinel, adds a static source lint test (test_openmp_optional.py), and replaces omp_get_wtime() with a system_clock-based get_wtime from the existing timing module so serial/non-GNU builds compile. I verified the sentinel mechanism compiles correctly under gfortran both with and without -fopenmp, and a repository-wide scan of 165 .f90 files finds no remaining unguarded omp_lib uses. However, the serial fallback added in simple_gpu.f90 silently drops particles in an OpenACC-enabled / OpenMP-disabled build — exactly the class of configuration this PR claims to enable.

Findings:

  1. [major] src/simple_gpu.f90:263–272 — With SIMPLE_ENABLE_OPENACC=ON (NVHPC) but ENABLE_OPENMP=OFF, ngpu can be > 1 (from SIMPLE_GPU_DEVICES/acc_get_num_devices), yet !$omp parallel is compiled out, so the block runs once serially with dev = 0. Then i1 = int(int(dev+1,8)*npart/ngpu) = npart/ngpu, so trace_orbits_gpu_range (a !$acc parallel loop kernel) only traces particles [1, npart/ngpu] and silently drops the rest. This config previously did not compile (the very bug this PR fixes); it now compiles but yields silently truncated GPU results. The serial no-OpenACC path is safe only because ngpu is pinned to 1 there. Fix: when the parallel region is disabled, fall back to the full range (mirror the ngpu <= 1 early return), e.g. i1 = npart in the serial case.

  2. [minor] test/python/test_openmp_optional.py:28–33 — The scanner globs only *.f90, not *.F90. src/util.F90 currently defines an #ifndef _OPENMP fallback omp_get_thread_num(), so nothing is missed today, but the stated contract ("any file that references an omp_lib symbol") is broader than the coverage; a future .F90 file with an unguarded omp_lib use would slip through. Recommend also globbing *.F90.

  3. [minor] test/python/test_openmp_optional.py + test/tests/CMakeLists.txt:54–61 — This is a static source lint, not an actual serial/OpenMP-off compile check, so it validates the "serial builds compile" claim only heuristically; the regex only recognizes use omp_lib and omp_(get|set|in|init|destroy|test)_...(...) call syntax. A real ENABLE_OPENMP=OFF build in CI (e.g. gfortran no-OpenMP make) would be stronger evidence and would have caught finding 1.

  4. [minor] src/simple_main.f90:879,897,905,908 — Replacing omp_get_wtime() with the system_clock-based get_wtime (src/timing.f90:52–62) loses the high-resolution, monotonic guarantees of omp_get_wtime; on coarse-resolution or wrapping clocks the SIMPLE_GPU_BENCH timings could be coarse or zero for fast traces. Acceptable for a benchmark, but worth documenting.

Verdict: Request changes — the !$ sentinel guarding is correct and well-tested, but the serial fallback in simple_gpu.f90 silently drops particles in the OpenACC-without-OpenMP configuration that this PR newly enables (finding 1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S review size up to 100 changed lines tier/T3 physics or output behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant