Guard omp_lib so serial and non-GNU builds compile - #514
Conversation
|
Test suite state, clean build tree ( 109/116 pass. All 7 failures reproduce on the parent commit
One worth flagging separately: |
There was a problem hiding this comment.
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:
-
[minor] python/pysimple/init.py:472 — the only real caller of
_needs_bminmax_cachestill passes only(params.num_surf, params.ntcut, params.class_plot)and omitsparams.fast_class, even though this PR added thefast_classparameter precisely to keep Python in sync with the Fortranclassification_enabled()predicate. It is currently harmless only because thenum_surf != 1fallback coincidentally yields the same result as the Fortrannum_surf > 1branch for everynum_surf, but it contradicts the PR's own "three call sites must agree" docstring and silently diverges the moment either predicate is touched. Fix: passparams.fast_class(or read it from params inside the function) so the Python cache gate tracks the Fortran dispatch. -
[minor] test/tests/test_class_scores_driver.py:31 — the oracle re-thresholds
jpar_spreadread back from the ASCIIclass_scores.datagainst a hardcodedTOL_PERPINV = 15.0, duplicatingtol_perpinv = 15.d0insrc/check_orbit_type.f90(and the underlyingdrift > tol_perpinvtest). 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.
88a1f5b to
8bd460d
Compare
|
Verified locally, and it surfaced a dependency. Serial build with
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 |
There was a problem hiding this comment.
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:
-
[major] src/simple_gpu.f90:263–272 — With
SIMPLE_ENABLE_OPENACC=ON(NVHPC) butENABLE_OPENMP=OFF,ngpucan be > 1 (fromSIMPLE_GPU_DEVICES/acc_get_num_devices), yet!$omp parallelis compiled out, so the block runs once serially withdev = 0. Theni1 = int(int(dev+1,8)*npart/ngpu) = npart/ngpu, sotrace_orbits_gpu_range(a!$acc parallel loopkernel) 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 becausengpuis pinned to 1 there. Fix: when the parallel region is disabled, fall back to the full range (mirror thengpu <= 1early return), e.g.i1 = npartin the serial case. -
[minor] test/python/test_openmp_optional.py:28–33 — The scanner globs only
*.f90, not*.F90.src/util.F90currently defines an#ifndef _OPENMPfallbackomp_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.F90file with an unguarded omp_lib use would slip through. Recommend also globbing*.F90. -
[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_libandomp_(get|set|in|init|destroy|test)_...(...)call syntax. A realENABLE_OPENMP=OFFbuild in CI (e.g. gfortran no-OpenMPmake) would be stronger evidence and would have caught finding 1. -
[minor] src/simple_main.f90:879,897,905,908 — Replacing
omp_get_wtime()with thesystem_clock-basedget_wtime(src/timing.f90:52–62) loses the high-resolution, monotonic guarantees ofomp_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).
Problem
Six files referenced
omp_libwithout the!$sentinel. That is a hard semantic error, not a warning, whenever the module is absent:-DENABLE_OPENMP=OFFon any compilerflangbuild on a toolchain whose OpenMP runtime ships noomp_lib.mod(LLVM 22 here does not; LLVM 23 does)Fix
Each site gets the sentinel plus a serial fallback:
simple_maintakes wall time fromtiming'sget_wtime(), which already documents itself as "replacement for omp_get_wtime", and routes the two debug prints through a guarded local thread id.simple_gpudefaults its device index to 0 — the same value the single-device path already implies whenngpu <= 1.classification,test_coord_transimport no symbols, so the sentinel alone suffices.Test
test/python/test_openmp_optional.pyrejects 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
ENABLE_OPENMP=OFFgfortran and flang-23 agree bit-for-bit on a Boozer-field case (midpoint integrator, 8 threads, pinned
start.dat):0.0e+00max 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_fieldignoresstartmodeand callsrandom_number, soisw_field_type = -1cases cannot be compared across compilers — gfortran and flang have different PRNGs. Any cross-compiler check needs a real field withstartmode = 2.