1687 fix: Make OpenMP no_create a documented no-op instead of aborting fypp. - #1724
Open
engineer-scientist wants to merge 3 commits into
Open
Conversation
OMP_NOCREATE_STR called #:stop whenever no_create was set. GPU_PARALLEL, GPU_PARALLEL_LOOP and GPU_DATA expand both the ACC and OMP directive strings at fypp time and let the compile-time #if select one, so the abort fired on OpenACC-only builds too -- making a documented parameter unusable on every backend. OpenMP has no no_create equivalent, and OMP_DEFAULT_STR already emits a present-by-default mapping, so emitting nothing is the closest safe behavior. Document the clause as OpenACC-only, matching the existing GPU_CACHE note. Generated Fortran is byte-identical across all 109 .fpp files in the three targets: every call site passes no_create=None, which already took the empty-string path. Closes MFlowCode#1687 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016xc51kikzrKdHDfXf8VV5p
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1724 +/- ##
=======================================
Coverage 61.21% 61.21%
=======================================
Files 84 84
Lines 21601 21601
Branches 3195 3195
=======================================
Hits 13223 13223
Misses 6209 6209
Partials 2169 2169 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OMP_NOCREATE_STR called #:stop whenever no_create was set. GPU_PARALLEL, GPU_PARALLEL_LOOP and GPU_DATA expand both the ACC and OMP directive strings at fypp time and let the compile-time #if select one, so the abort fired on OpenACC-only builds too -- making a documented parameter unusable on every backend.
OpenMP has no no_create equivalent, and OMP_DEFAULT_STR already emits a present-by-default mapping, so emitting nothing is the closest safe behavior. Document the clause as OpenACC-only, matching the existing GPU_CACHE note.
Generated Fortran is byte-identical across all 109 .fpp files in the three targets: every call site passes no_create=None, which already took the empty-string path.
Closes #1687
Claude-Session: https://claude.ai/code/session_016xc51kikzrKdHDfXf8VV5p
Description
OMP_NOCREATE_STRcalled#:stop 'no_create is not supported yet'wheneverno_createwas non-None. BecauseGPU_PARALLEL,GPU_PARALLEL_LOOPandGPU_DATAbuild both the ACC and OMP directive strings with#:setand then let the compile-time#if defined(MFC_OpenACC)/#elif defined(MFC_OpenMP)pick one, that#:stopfired during fypp preprocessing on OpenACC-only builds too — making a documented public parameter unusable on every backend, with an error pointing intoomp_macros.fpprather than at the caller.This takes option (1) from the issue: the OpenMP path becomes a documented no-op, and the working OpenACC implementation (
GEN_NOCREATE_STR) stays reachable.Worth noting for context:
MFC_OpenACC/MFC_OpenMPare never passed to fypp —cmake/Fypp.cmakeonly definesMFC_<CompilerId>,MFC_<TARGET>,MFC_COMPILER,MFC_CASE_OPTIMIZATIONandchemistry. So fypp cannot know which backend will survive, and expanding both is structural rather than incidental. The invariant that follows is that every clause helper must be evaluable for every build; a#:stopin one backend's helper is an unconditional abort, not a guard.Emitting nothing is the closest safe behavior rather than a silent wrong answer: OpenACC's
no_createasks for "use the device copy if present, otherwise the host copy, don't allocate", andOMP_DEFAULT_STRalready emitsdefaultmap(present:allocatable)(Cray) /defaultmap(tofrom:...)— the present-by-default behavior is already ambient.Also adds
(OpenACC only)to theno_createrow of the three parameter tables ingpuParallelization.md, matching the existingGPU_CACHEconvention ("NOTE Does not do anything for OpenMP currently"). Without it, the fix trades a loud failure for a silent one.The replacement comment is deliberately the same line count as the code it replaces, so fypp's
# linemarkers don't shift and the generated Fortran stays byte-identical (see Testing).Closes #1687.
Type of change (delete unused ones)
Testing
MFC_SIMULATIONonly, no OpenMP anywhere:no_create='[a]'on each):Macro OpenACC OpenMP
GPU_PARALLEL!$acc parallel default(present) no_create(a)!$omp target teams defaultmap(tofrom:aggregate) …GPU_DATA!$acc data no_create(a)!$omp target dataGPU_PARALLEL_LOOP!$acc parallel loop gang vector default(present) private(i) no_create(a)!$omp target teams loop … private(i).fppin the tree twice (original vs. patchedomp_macros.fpp), using each target's real generated includes and the exact flag set fromcmake/Fypp.cmake, and diffed the generated Fortran:Target Files Result
pre_process29 byte-identicalsimulation57 byte-identicalpost_process23 byte-identicalAlso repeated for
chemistry=Trueand for--case-optimization+chemistry=True— byte-identical in every configuration.(Only fypp's embedded absolute source path differs between the two checkouts; normalized away.) Expected, since every call site passes
no_create=Noneand already took the#:set no_create_val = ''path — but demonstrated rather than asserted../mfc.sh format— 294 files unchanged./mfc.sh precheck— 7/7 passed./mfc.sh build—pre_process,simulation,post_processall build./mfc.sh test— 609/611 pass. Two chemistry cases failed:2D -> Chemistry -> Reacting Mixing Layer(C4EB58A8, tolerance) and2D -> Chemistry -> Spatial Reacting Mixing Layer(56F8C4BC,pre_processexit 1). I don't believe these are mine, and I checked rather than assumed: both pass when run in isolation with this patch applied, and both pass on stock master. Point 3's expansion diff also holds for their exact build configuration — byte-identical underchemistry=True(29 + 57 files) and under--case-optimization+chemistry=True(39 files) — so the binaries under test are unchanged by this patch. 56F8C4BC's error ishcid=274 file has more lines than the grid: examples/2D_spatial_reacting_mixing_layer/IC/prim.1.00.000000.dat, i.e. a shared example input read against the harness's shrunken 31×31 grid, which reads like interference in the parallel full-suite run. Caveat: I did not re-run the full 41-minute suite on master to reproduce them there, so I'm flagging rather than claiming a known flake — happy to dig further if you'd like.I have no GPU or ACC-capable compiler, so the emitted
no_create(...)syntax has not been compiled by an OpenACC compiler — though it is the sameGEN_PARENTHESES_CLAUSEoutput as every other ACC clause here, and nothing in the tree passesno_createat runtime today.No test added.
toolchain/mfc/test/cases.pygenerates end-to-end solver runs compared against golden files; there is no unit layer for macro expansion, and ano_createcase would require inventing a solver call site that passes the parameter. Happy to add a fypp-expansion smoke test as new infrastructure if you'd like it, but that felt like its own PR.Checklist
Check these like this
[x]to indicate which of the below applies.See the developer guide for full coding standards.
GPU changes (expand if you modified
src/simulation/)Audit of the other clauses (per the issue's closing question)
You asked whether
create,attach,deviceptrandpresentshare the same trap. I checked every#:stopin the macro layer:createOMP_CREATE_STR→map(always,alloc:)presentOMP_PRESENT_STR→map(present,alloc:)deviceptrOMP_DEVICEPTR_STR→use_device_ptr(...)attachOMP_PARALLEL*default='none'OMP_DEFAULT_STR(omp_macros.fpp:36)defaultis public and documented as accepting'present'or'none';GEN_DEFAULT_STRhandles'none'correctly butOMP_DEFAULT_STR#:stops on it, soGPU_PARALLEL_LOOP(default='none')aborts fypp on an OpenACC-only build for exactly the same reason.no_create, emitting nothing fordefault='none'would be a semantic change rather than a no-op —default(none)demands explicit data clauses, and silently dropping it would weaken a correctness check the caller asked for. Happy to open a separate issue, or to fold a fix in here if you'd prefer.#:stops (shared_parallel_macros.fpp:32,34,92,omp_macros.fpp:262,319) all reject malformed input — unbalanced parentheses, a reduction list without an operator, aGPU_DATAwrapping no code. Those are backend-independent and correct as-is.AI code reviews
Reviews are not retriggered automatically. To request a review, comment on the PR:
@claude full review— Claude full review (also triggers on PR open/reopen/ready)claude-full-review— Claude full review via label