From 23e75f058f96cea967bbd909f27ebf63fa6aa016 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Fri, 11 Sep 2026 22:08:14 -0500 Subject: [PATCH 1/3] chore: consolidate agent instructions into .claude/CLAUDE.md Replace the root CLAUDE.md and .claude/rules/common-pitfalls.md with a single .claude/CLAUDE.md. Add AGENTS.md as a symlink to it so Codex loads the same file with no per-user configuration; a repo-level .codex/config.toml is not read by Codex. Written with assistance from Claude Code. --- .claude/CLAUDE.md | 15 +++ .claude/rules/common-pitfalls.md | 173 ------------------------------- AGENTS.md | 1 + CLAUDE.md | 147 -------------------------- 4 files changed, 16 insertions(+), 320 deletions(-) create mode 100644 .claude/CLAUDE.md delete mode 100644 .claude/rules/common-pitfalls.md create mode 120000 AGENTS.md delete mode 100644 CLAUDE.md diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md new file mode 100644 index 0000000000..a6913c6d01 --- /dev/null +++ b/.claude/CLAUDE.md @@ -0,0 +1,15 @@ +* New branches cannot be made on MFlowCode/MFC, they are made on forks +* PRs: + * made using AI tools like Claude Code and Codex should say so. + * are made from those MFC forks + * that change CFD result need verification PR is correct + * follow template + * that break a feature but promise a followup PR to fix it are rejected +* Commands: + * MFC should almost always build and run using the ./mfc.sh command + * Running mfc.sh commands can create a lock file in build/ that is sticky, be careful +* Programming and Design: + * New code should follow the DRY principle and also make side-effect code DRY as well + * Comments should be as short as possible without sacrificing value + * GPU macros should follow existing the source's GPU macro principles and patterns + * Functions/subroutines/modules shorter is better while being correctness, fast, and separating concerns diff --git a/.claude/rules/common-pitfalls.md b/.claude/rules/common-pitfalls.md deleted file mode 100644 index b3d7b0e47b..0000000000 --- a/.claude/rules/common-pitfalls.md +++ /dev/null @@ -1,173 +0,0 @@ -# Common Pitfalls - -Traps that live in no other doc, collected because their failure modes are silent — -wrong answers and wrong indices, not error messages. General development pitfalls are -covered in `docs/documentation/contributing.md`. - -## Indexing and Ghost Cells - -- Grid dimensions `m`, `n`, `p` (cells in x, y, z); 1D: n=p=0, 2D: p=0. Interior `0:m`; - ghost region `-buff_size:m+buff_size`; bounds structs `idwint(1:3)` (interior) and - `idwbuff(1:3)` (with ghosts); cell boundaries `x_cb(-1-buff_size:m+buff_size)`. -- `buff_size` is **not** a single formula: it's set per reconstruction scheme in - `s_configure_coordinate_bounds` (`m_helper_basic.fpp`) and floored higher for Lagrange - bubbles and IB. Read that routine for the current value rather than assuming one. -- Riemann solvers: left state at `j`, right state at `j+1`. -- All equation indices live in the `eqn_idx` struct (`eqn_idx_info` in - `m_derived_types.fpp`, populated by `s_initialize_eqn_idx` in `m_global_parameters_common.fpp`): `%cont`, `%mom`, `%E`, - `%adv`, plus optional ranges (`%bub`, `%stress`, `%species`, `%B`, ...). The old - `contxb`/`momxb` shorthands are gone. Index positions depend on `model_eqns` and - enabled features — changing either moves ALL indices; never hard-code one. - -## GPU - -- WARNING: do NOT wrap `GPU_LOOP` in `GPU_PARALLEL` for spatial loops — `GPU_LOOP` emits - empty directives on Cray and AMD, causing silent serial execution. Spatial loops always - use `GPU_PARALLEL_LOOP`/`END_GPU_PARALLEL_LOOP`. Macro API: - `docs/documentation/gpuParallelization.md`; signatures: - `src/common/include/parallel_macros.fpp`. Never call the `ACC_*`/`OMP_*` - implementation layers directly. -- Only `src/simulation/` is GPU-accelerated. Backends: OpenACC (nvfortran primary, Cray) - and OpenMP offload (Cray primary, AMD flang, nvfortran). The CPU-only build must always - work — every `#ifdef` needs a path for all configurations (CPU, ACC, OMP, with/without - MPI). Gates: `MFC_GPU`, `MFC_OpenACC`, `MFC_OpenMP`, `MFC_MPI`, `MFC_DEBUG`, - `MFC_SINGLE_PRECISION`/`MFC_MIXED_PRECISION`, `MFC_PRE_PROCESS`/`MFC_SIMULATION`/ - `MFC_POST_PROCESS`, and compiler macros (`_CRAYFTN`, `__PGI`, ...). -- `@:ACC_SETUP_VFs(...)`/`@:ACC_SETUP_SFs(...)` GPU pointer setup compiles only under - Cray. Around MPI: `GPU_UPDATE(host=...)` before send, `GPU_UPDATE(device=...)` after - receive. -- An array whose bound is a device global (`dimension(num_fluids)`, `dimension(num_species)`) may be - passed to a device routine **from a parallel-loop body, but not from inside another - `GPU_ROUTINE(parallelism='[seq]')`**. CCE OpenACC rejects the second form with - `ftn-7066 ... Global in accelerator routine without declare -- num_fluids`, and reports it at - whatever line it gave up on: remove one trigger and the message *walks forward* to the next call, - so the reported line is not the cause. Only the plain lanes fail - under `--case-optimization` - those bounds are `parameter`s, so a green Case Opt lane beside a failing plain one is the - signature. Every accepted call site in the tree already obeys this (`m_cbc`, `m_ibm`, - `m_bubbles_EL`, `s_compute_cell_state`): form such a call in the loop body and pass scalars - deeper. Neither `cray_inline` nor a `num_fluids_max` bound nor dropping optional dummies helps - - all three were measured. -- nvfortran 23.11/24.1 segfault (`fort2 TERMINATED by signal 11`) on a caller that passes a - `parameter` array from `m_thermochem` (e.g. `molecular_weights`) into a declare-target routine. - Read such arrays directly in the kernel, or pass a plain local computed from them. -- The `USING_AMD` fypp guards (86 sites, `#:set` in `src/common/include/shared_parallel_macros.fpp`) are - load-bearing, not a stale workaround - do not "modernize" them away. They swap a device-global array - bound for a literal: `dimension(3)` for `num_dims`/`num_fluids` when case optimization is off (64 - sites), and `dimension(20)` for `sys_size` in `m_compute_cbc` (21 sites, with a matching - `@:PROHIBIT` in `m_start_up` capping `sys_size <= 20` under AMD+CBC). Setting `USING_AMD = False` - and rebuilding amdflang `--gpu mp` without case optimization compiles CLEAN - 728 s, zero - diagnostics - and then NaNs at step 50 in CBC, riemann `wave_speeds=2`, IBM, surface tension, - QBMM/viscous and MHD HLLD, while both Lagrange bubble cases *complete* with out-of-tolerance - answers. Measured 2026-08-29 on MI210. A compile-only check returns green, so any future attempt to - drop these must run the tests, not just build. -- **CCE OpenACC (19.0.0 through 21.0.2, `-O2`; `-O0`/`-O1` correct; OpenMP offload unaffected): a device - routine that contains any `GPU_LOOP` (itself or in anything it calls) must be called with scalars, - never with an array element as an actual argument.** Every `routine` level is affected, including the - conforming `loop vector` inside `routine vector`. With both ingredients present the - element is misaddressed: an `intent(in)` element reads as garbage, an `intent(out)` element is - never written. Either ingredient alone is fine, which is why master's `s_compute_pressure(q%sf(j,k,l),...)` - works (no loop) and `s_compute_mixture_coefficients` works (scalar actuals). PR #1811 added the - Newton and RK4 loops to the EOS helpers and every call that passed `%sf(j,k,l)` or `blkmod1(k,l,q)` - ended in `NaN(s) in timestep output` on the Frontier CCE OpenACC lanes only, bit-identical on every - other backend. Fix: copy elements to locals before the call, receive into a local. 37-line - reproducer and the bisection: sbryngelson/compiler-bugs `cce/acc-routine-element-by-reference`, - MFC #1815. Do not "fix" it by deleting the `seq` directives instead: they are the idiom master - uses in every device routine. -- The same "call it from the loop body" rule covers `m_thermochem`: calling `get_species_*` from - inside a `GPU_ROUTINE` rather than from the kernel gave CCE OpenMP a runtime - `Memory access fault by GPU node-N ... Reason: Unknown` on the first step (exit 134), while every - other backend ran. Evaluate them at the call site and pass the arrays in. Note this one only shows - at runtime, and only on a case that reaches the path - the build is clean. - -## Parameters - -- Adding one: `_r()` definition + `_nv()` `NAMELIST_VARS` registration in - `toolchain/mfc/params/definitions.py`; `case_validator.py` only if physics-constrained - (with a `PHYSICS_DOCS` entry). Fortran declarations and namelist bindings are - auto-generated at build time (ninja-tracked custom command) — re-run cmake (or `./mfc.sh build`) after editing. -- Still manual: derived-type `TYPE` member definitions in `src/common/m_derived_types.fpp`; - default-value assignments in `s_assign_default_values_to_user_inputs`; the - `CASE_OPT_EXTRA_LINES` literal in `toolchain/mfc/params/generators/fortran_gen.py` (covers `num_dims`, - `num_vels`, `weno_polyn`, `muscl_polyn`, `weno_num_stencils`, `wenojs`); - multi-variable declaration lines (`bc_x/y/z`, `x/y/z_domain`, `x/y/z_output`, post's - `G`); and the MPI broadcast residue in `m_mpi_proxy` (computed variables that are not - namelist-bound: `m_glb`/`n_glb`/`p_glb`, `cfl_dt`, `bc_io`, and complex struct-member - array loops — these cannot be auto-generated and stay hand-listed). Everything else — scalar declarations, plain arrays (`FORTRAN_ARRAY_DIMS` - table in `definitions.py`), derived-type namelist declarations including `GPU_DECLARE` - lines and Doxygen descs (`TYPED_DECLS` table in `definitions.py`), the simulation - case-optimization declaration block, and the per-target MPI broadcast lists for all - namelist-registry scalars (`generated_bcast.fpp`) — is regenerated at build time by a - ninja-tracked custom command (editing `params/*.py` triggers regeneration automatically). - Gotcha: ADDING a new file under `toolchain/mfc/params/` needs one reconfigure - (the custom command's DEPENDS list is globbed at configure time). Under `--case-optimization` the baked-in constants are dropped from the - namelist, so changing one needs a *rebuild*, not a case edit. -- Derived-type params (`chem_params`, `lag_params`, `rburn`) are NOT auto-broadcast: - `generated_bcast.fpp` covers namelist *scalars* only. Each type needs a hand-written - `_emit_` in `toolchain/mfc/params/generators/fortran_gen.py` plus its call site in - the `target == "sim"` block, and — if it is read on device — an explicit - `$:GPU_UPDATE(device='[name]')` in BOTH `m_global_parameters.fpp` and `m_start_up.fpp` - (`GPU_DECLARE` alone does not make it device-resident). Regrouping scalars into a derived - type silently drops their broadcast, so every non-root rank keeps the `dflt_real` - sentinel; single-rank goldens cannot see this, so pair such a change with a `ppn=2` test - and confirm it fails without the emitter. -- A `patch_ib` member that any `m_ibm` ghost-point code reads must ALSO be set in - `s_add_cloud_particle` (`src/simulation/m_particle_cloud.fpp`): `particle_cloud_ibs` is - `allocate`d without default initialization, and `s_reduce_ib_patch_array` copies the whole - struct into `patch_ib`, overwriting the defaults from - `s_assign_default_values_to_user_inputs`. Anything left unset reaches the solver as - uninitialized memory, and only where the allocation is not already zero-filled — a - garbage `v_blow` failed Frontier AMD with `ICFL is NaN` while every NVIDIA lane and all - local CPU/GPU runs passed. A platform-only NaN is the signature of this class. -- Shared-state pattern: namelist declarations (`#:include 'generated_decls.fpp'`), the - `eqn_idx`/`sys_size` state variables, and the common defaults - core all live in `src/common/m_global_parameters_common.fpp`. Each per-target - `m_global_parameters.fpp` does `use m_global_parameters_common` (default-public), so - `use m_global_parameters` continues to work for all downstream modules without change. - `src/common/` carries no `MFC_PRE_PROCESS`/`MFC_SIMULATION`/`MFC_POST_PROCESS` guards: - stage-varying behavior is passed in as an explicit argument or initialization policy, and - device residency for generated simulation scalars is emitted from `SIM_GPU_DECL_VARS` - (`toolchain/mfc/params/generators/fortran_gen.py`). Generated includes - (`generated_decls.fpp`, `generated_bcast.fpp`, `generated_case_opt_decls.fpp`) must exist for every target — pre/post - get the common computed scalars (`num_dims`, `num_vels`, `weno_polyn`, `muscl_polyn`), so - a common file that includes one will compile for pre/post too. -- Runtime checks (`@:PROHIBIT`) go where they run: shared → - `src/common/m_checker_common.fpp`; simulation-only → `src/simulation/m_checker.fpp`; - pre/post-only → `src/{pre,post}_process/m_checker.fpp` (their `s_check_inputs` are - currently empty — that IS the right place, not m_checker_common). -- Analytic ICs are compiled into the binary. Expressions are AST-validated at case load - (syntax errors and unknown variables are immediate, named errors; bare `e` is not a - variable — write `exp(1.0)`). - Each IC variable maps to an `eqn_idx%…` expression in `QPVF_IDX_VARS` - (`toolchain/mfc/case.py`); a new patch-settable conserved variable means updating that - map AND the Fortran `eqn_idx` builder to agree — a mismatch is a silent wrong index. - Variables available in expressions: `docs/documentation/case.md`. - -## Tests - -- Tests are generated programmatically in `toolchain/mfc/test/cases.py` (parameter - modifications on `BASE_CFG` via the `CaseGeneratorStack` push/pop pattern); test UUID = - CRC32 of the trace string; `./mfc.sh test -l` lists all. -- `--only` matches whole trace *elements*, not substrings, and `_filter_only` - (`toolchain/mfc/test/test.py`) **ANDs labels while ORing UUIDs**. So `--only bubbles` matches - nothing (the element is `Bubbles`), and `--only low_Mach=1 low_Mach=2` asks for cases carrying - both and also matches nothing. It then exits **143**, which reads like an external kill rather - than an empty filter. Pass UUIDs whenever you want the union of several groups. -- Sibling `define_case_d` calls off the same stack level are never *combined*. Two switches that - only matter together (`avg_state=1` needs `wave_speeds=2` to be read at all) therefore get zero - effective coverage unless something pushes one and defines the other beneath it. Check - reachability before trusting that a flag is tested. -- `--no-build` silently runs whatever binary is on disk for a configuration it did not build. - Chemistry has its own config (`gpu-mp-chem-*`) that a plain `./mfc.sh build` never produces, so - a `--no-build` run reports failures from stale binaries and hides real compile breaks. Run - chemistry-touching sets without it. -- Pick the newest binary by the *binary's* mtime (`ls -t build/install/*/bin/simulation`), not the - install directory's - a stale config's directory can be newer than a fresh build's. -- The pre-commit hook lives in the main repo's `.git/hooks/` and git exports `GIT_DIR` there - during a commit, so from a worktree the toolchain lint enumerates the *other* checkout and - fails. Reproduce with `GIT_DIR=
/.git ./mfc.sh precheck`. Run precheck by hand and commit - with `--no-verify`. -- `/tmp` is node-local: scratch does not survive a compute-node change, and its absence is - silence, not an error. Keep patches and resource baselines on a shared filesystem. -- Golden files are tolerance-compared. Regenerate only the affected tests - (`./mfc.sh test --generate --only `) — an unexplained golden-file diff is a bug - report, not noise to be regenerated away. diff --git a/AGENTS.md b/AGENTS.md new file mode 120000 index 0000000000..ac55cbdc9c --- /dev/null +++ b/AGENTS.md @@ -0,0 +1 @@ +.claude/CLAUDE.md \ No newline at end of file diff --git a/CLAUDE.md b/CLAUDE.md deleted file mode 100644 index b0c165ddde..0000000000 --- a/CLAUDE.md +++ /dev/null @@ -1,147 +0,0 @@ -# MFC — Multi-component Flow Code - -MFC is an exascale multi-physics CFD solver written in modern Fortran 2008+ with Fypp -preprocessing. It has three executables (pre_process, simulation, post_process), a Python -toolchain for building/running/testing, and supports GPU acceleration via OpenACC and -OpenMP target offload. It must compile with gfortran, nvfortran, Cray ftn, and Intel ifx (CI-gated). -AMD flang is additionally supported for OpenMP target offload GPU builds. - -## How to Work Here - -You are editing this repository as a conservative maintainer. Three facts shape everything: - -1. **Four compilers, one truth.** Every line must compile and behave identically under four - CI-gated compilers and three GPU configurations. Code that is merely clever on one is - broken on another. Prefer the established idiom over the elegant one. -2. **Failures here are silent.** The worst bugs in a CFD code are not crashes — they are - answers that are subtly wrong: a wrong index, a serial loop that should be parallel, a - precision mix. Golden-file regression tests are the safety net, and incidental edits are - how regressions slip past them. -3. **Trust the toolchain.** `./mfc.sh` already solves environments, module loading, - dependencies, and build configuration; the lint (`./mfc.sh precheck`) encodes the - project's forbidden patterns. When unsure of a flag or rule, ask the tooling - (`--help`, precheck) rather than guessing. - -**Primary rule: make the smallest correct change. Prefer deleting code to adding code.** - -Diff discipline: -- Every changed line should trace to the request. Keep changes localized. -- Do not rewrite, reformat, or rename unrelated code. -- Do not add new files, dependencies, or net-positive LOC without clear justification. - -Do not add bloat: -- abstractions for one call site, or defensive code for hypothetical future callers -- validation layers around trusted internal data — constraint checks belong in - `case_validator.py` and the `m_checker*.fpp` files, not scattered through the solver -- optional parameters or config knobs for behavior with one correct value -- broad try/except blocks in toolchain Python -- comments explaining obvious code; compatibility shims unless explicitly requested - -Tests: add one only when it protects real behavior — it would fail before your change and -covers behavior a real case depends on. Prefer one targeted case over many broad ones. - -Before editing, state: the smallest viable fix, what changes, what deliberately does not -change, and whether a test is needed. After editing, report: files changed, net LOC, and -anything that could still be deleted or simplified. If a patch exceeds roughly 100 net new -lines, stop and justify before continuing. - -For general behavioral guidance (simplicity, surfacing assumptions, verifiable success -criteria), invoke the `karpathy-guidelines` skill. - -## Commands - -All commands run from the repo root via `./mfc.sh`; avoid invoking CMake, Python toolchain -scripts, or Fortran compilers directly. Run `./mfc.sh --help` for flags -(`-j N` = parallel jobs). - -```bash -./mfc.sh build -j 8 # all 3 targets; -t , --gpu acc|mp, --debug -./mfc.sh run case.py -n 4 # run with 4 MPI ranks; -e batch for clusters -./mfc.sh test -j 8 # full suite; --only , -l to list, - # --generate to refresh golden files after an intended output change -./mfc.sh format -j 8 # auto-format Fortran + Python -./mfc.sh precheck -j 8 # all CI lint checks — run before every commit -./mfc.sh validate case.py # validate a case without running -./mfc.sh params # search case parameters -``` - -On HPC clusters, load modules before building: `source ./mfc.sh load -c -m ` -(the `source` is required). Slugs and per-system GPU backends: `toolchain/modules`; batch -templates: `toolchain/templates/`. Identify the system via `$LMOD_SYSHOST`, then a -non-empty `$CRAY_LD_LIBRARY_PATH` (→ Cray), then `hostname`. - -## Development Workflow Contract - -For ALL code changes: read the relevant code first, plan multi-file changes before -implementing, then **format → precheck → build → test → commit**, in that order, with one -logical change per commit. - -- YOU MUST run `./mfc.sh precheck` before any commit (pre-commit hooks enforce this). -- YOU MUST run the tests relevant to your change before claiming work is done. - Changes to `src/common/` are shared by all three executables — test all three. -- NEVER commit code that does not compile or fails tests. -- NEVER use heredocs for git commit messages. Use simple `git commit -m "message"`. - -## Architecture - -``` -src/common/ # shared by ALL three executables — wide blast radius -src/pre_process/ # grid generation and initial conditions -src/simulation/ # CFD solver (the only GPU-accelerated target) -src/post_process/ # data output and visualization -toolchain/ # Python CLI; params/definitions.py is the parameter source of truth -examples/ # example cases (case.py); tests/ holds regression golden files -``` - -Source files are `.fpp` (Fortran + Fypp macros), preprocessed to `.f90` by CMake. - -## Critical Rules - -The exhaustive forbidden-pattern list is `toolchain/mfc/lint_source.py` (enforced by -precheck and CI). The rules to internalize — they exist because MFC must behave -identically across compilers, precisions, and GPU backends: - -- GPU directives only via `GPU_*` Fypp macros — NEVER raw `!$acc`/`!$omp` pragmas. - (Raw `#ifdef`/`#ifndef` guards for feature/compiler/library gating ARE normal.) -- Precision only via `wp`/`stp` kinds and generic intrinsics — NEVER `dsqrt`/`dble`/ - `real(8)` or `d` exponent literals. Write `sqrt`, `1.0_wp`, `real(..., wp)`. -- Abort only via `call s_mpi_abort()` or `@:PROHIBIT()`/`@:ASSERT()` — NEVER `stop`/`error stop`. -- NEVER `goto`, `COMMON` blocks, or global `save` variables. -- Every `@:ALLOCATE(...)` MUST have a matching `@:DEALLOCATE(...)`. -- New parameters are defined in `toolchain/mfc/params/definitions.py` (plus - `case_validator.py` if physics-constrained); Fortran declarations and namelist bindings - are auto-generated. Exceptions: `.claude/rules/common-pitfalls.md`. - -## Naming and Style - -Modules `m_` with `s_initialize_/s_finalize__module` pairs; public -subroutines `s__`, functions `f__`; 2-space indent, lowercase -keywords, explicit `intent` on all arguments; constants get descriptive names, not -ALL_CAPS. Full hard/soft rule tables: `docs/documentation/contributing.md`. - -## Precision - -`wp` = working precision (computation); `stp` = storage precision (field arrays and I/O). -Both double by default; `--single` → both single; `--mixed` → wp=double, stp=half — so -wp/stp mixing is a silent precision bug, not a style issue. `scalar_field%sf` and all new -field arrays use `stp`. MPI types must match: `mpi_p` ↔ `wp`, `mpi_io_p` ↔ `stp`. - -## Where Things Are Documented - -`docs/documentation/` is freshness-checked by precheck (`lint_docs.py`) — prefer pointing -there over restating it. Most relevant: `contributing.md` (standards, architecture, -general pitfalls), `gpuParallelization.md` (GPU macro API), `testing.md` (test system), -`case.md` (case parameters, analytic ICs). MFC-specific traps with silent failure modes -live in `.claude/rules/common-pitfalls.md` — read it before touching indexing, GPU loops, -parameters, or tests. - -## Code Review Priorities - -When reviewing PRs, prioritize in this order: -1. Correctness (logic bugs, numerical issues, array bounds) -2. Precision discipline (stp vs wp mixing) -3. Memory management (@:ALLOCATE/@:DEALLOCATE pairing, GPU pointer setup) -4. MPI correctness (halo exchange, buffer sizing, GPU_UPDATE calls) -5. GPU code (GPU_* Fypp macros only, no raw pragmas) -6. Physics consistency (pressure formula matches model_eqns) -7. Compiler portability (4 CI-gated compilers + AMD flang for GPU) From 1e643af4a00a395b2af7446ce67c65e3ae92cc22 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Fri, 11 Sep 2026 22:11:34 -0500 Subject: [PATCH 2/3] chore: repoint instruction-file references at .claude/CLAUDE.md The review workflow read the now-removed root CLAUDE.md and cited its Code Review Priorities section; both reads were guarded with '|| true', so the review step would have run with empty standards rather than failing. lint_source.py pointed contributors at the removed .claude/rules/common-pitfalls.md; its docstring already carries the full explanation, so the pointer now names the compiler-bugs reproducer only. Written with assistance from Claude Code. --- .github/workflows/claude-code-review.yml | 6 +++--- toolchain/mfc/lint_source.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index d5fc7a32d3..9da94b98d5 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -206,7 +206,7 @@ jobs: Hard scope rules: - Do NOT inspect checked-out repository code except: - - ./CLAUDE.md + - ./.claude/CLAUDE.md - ./.claude/rules/*.md (max 10 files) - ${{ steps.review_input.outputs.review_diff_path }} - ${{ steps.review_input.outputs.changed_files_path }} @@ -225,7 +225,7 @@ jobs: Allowed workflow: 1) ls -1 .claude/rules 2>/dev/null || true - 2) cat CLAUDE.md 2>/dev/null || true + 2) cat .claude/CLAUDE.md 2>/dev/null || true 3) find .claude/rules -maxdepth 1 -name "*.md" -print | head -n 10 | xargs -I{} cat "{}" 2>/dev/null || true 4) cat "${{ steps.review_input.outputs.changed_files_path }}" 5) cat "${{ steps.review_input.outputs.review_diff_path }}" @@ -238,7 +238,7 @@ jobs: - Do NOT restate the full PR summary. - If there are no high-confidence findings, leave .claude-review/output.md empty and STOP. - Review standard (in priority order per CLAUDE.md "Code Review Priorities"): + Review standard (in priority order): 1. Correctness (logic bugs, numerical issues, array bounds) 2. Precision discipline (stp vs wp mixing) 3. Memory management (@:ALLOCATE/@:DEALLOCATE pairing, GPU pointer setup) diff --git a/toolchain/mfc/lint_source.py b/toolchain/mfc/lint_source.py index 494f260871..7dca078f06 100644 --- a/toolchain/mfc/lint_source.py +++ b/toolchain/mfc/lint_source.py @@ -630,7 +630,7 @@ def check_device_routine_element_args(repo_root: Path) -> list[str]: the element as garbage and never writes it back. Either alone is fine, every `routine` level is affected, and the loop counts when it sits in anything the routine calls. Copy the element to a scalar before the call and receive results into a scalar. See - .claude/rules/common-pitfalls.md and sbryngelson/compiler-bugs cce/acc-routine-element-by-reference. + sbryngelson/compiler-bugs cce/acc-routine-element-by-reference. """ src_dir = repo_root / SRC_DIR files = {src: src.read_text(encoding="utf-8").splitlines() for src in _fortran_fpp_files(src_dir)} @@ -683,7 +683,7 @@ def check_device_routine_element_args(repo_root: Path) -> list[str]: for arg in _split_top_level(stmt[m.end() : j - 1]): e = _ELEMENT_ARG.match(arg) if e and ":" not in arg and not _VALUE_CALL_NAMES.match(e.group(1)): - errors.append(f" {rel}:{line_no} `{arg}` into `{name}` (a device routine with a seq loop): pass a scalar, see common-pitfalls.md") + errors.append(f" {rel}:{line_no} `{arg}` into `{name}` (a device routine with a seq loop): pass a scalar, see sbryngelson/compiler-bugs cce/acc-routine-element-by-reference") return errors From 9dabc277775875f9c39addc722cbb4344ee7f665 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Fri, 11 Sep 2026 22:19:20 -0500 Subject: [PATCH 3/3] docs: relocate the common-pitfalls material into the user docs The removed .claude/rules/common-pitfalls.md held traps whose failure modes are silent. Its content is redistributed to the docs page that owns each subject rather than to a new page: indexing/ghost-cell and parameter-plumbing traps into contributing.md's Common Pitfalls section, compiler and backend traps into a new Silent-Failure Traps section in gpuParallelization.md, and test-selection traps into testing.md. Material already covered in those pages (Riemann j/j+1 indexing, the add-a-parameter procedure and its still-manual list, the AMD case-opt bound pattern) was dropped rather than duplicated. The testing.md --only bullet described substring matching and is corrected to whole-element matching. Written with assistance from Claude Code. --- docs/documentation/contributing.md | 11 ++++++ docs/documentation/gpuParallelization.md | 49 ++++++++++++++++++++++++ docs/documentation/testing.md | 34 +++++++++++++++- 3 files changed, 93 insertions(+), 1 deletion(-) diff --git a/docs/documentation/contributing.md b/docs/documentation/contributing.md index 7e13e4179e..d42dc09e87 100644 --- a/docs/documentation/contributing.md +++ b/docs/documentation/contributing.md @@ -175,6 +175,9 @@ Both human reviewers and AI code reviewers reference this section. - MFC uses **non-unity lower bounds** (e.g., `idwbuff(1)%%beg:idwbuff(1)%%end` with negative ghost-cell indices). Always verify loop bounds match array declarations. - **Riemann solver indexing:** Left states at `j`, right states at `j+1`. Off-by-one here corrupts fluxes. +- **Grid extents:** `m`, `n`, `p` are cell counts in x, y, z (1D sets `n = p = 0`, 2D sets `p = 0`). The interior is `0:m`, the ghost region `-buff_size:m+buff_size`, and cell boundaries run `x_cb(-1-buff_size:m+buff_size)`. Bounds are carried in `idwint(1:3)` (interior) and `idwbuff(1:3)` (with ghosts). +- **`buff_size` is not a single formula.** It is set per reconstruction scheme in `s_configure_coordinate_bounds` (`src/common/m_helper_basic.fpp`) and floored higher for Lagrange bubbles and immersed boundaries. Read that routine rather than assuming a value. +- **Never hard-code an equation index.** They live in the `eqn_idx` struct (`eqn_idx_info` in `src/common/m_derived_types.fpp`, populated by `s_initialize_eqn_idx` in `src/common/m_global_parameters_common.fpp`): `%%cont`, `%%mom`, `%%E`, `%%adv`, plus the optional ranges `%%bub`, `%%stress`, `%%species`, and `%%B`. Index positions depend on `model_eqns` and on which features are enabled, so changing either moves every index. ### Precision and Type Safety @@ -212,6 +215,14 @@ Both human reviewers and AI code reviewers reference this section. - CLI schema in `toolchain/mfc/cli/commands.py` must match argument parsing. - Check subprocess calls for shell injection risks and missing error handling. +### Parameter Plumbing + +- **Derived-type parameters are not auto-broadcast.** `generated_bcast.fpp` covers namelist *scalars* only. Each derived type (`chem_params`, `lag_params`, `rburn`) needs a hand-written `_emit_` in `toolchain/mfc/params/generators/fortran_gen.py` plus its call site in that generator's simulation branch, and, if it is read on device, an explicit `$:GPU_UPDATE(device='[name]')` in both the target's `m_global_parameters.fpp` and `src/simulation/m_start_up.fpp` — `GPU_DECLARE` alone does not make it device-resident. Regrouping existing scalars into a derived type silently drops their broadcast, leaving every non-root rank holding the `dflt_real` sentinel. Single-rank golden files cannot catch this, so pair such a change with a `ppn=2` test and confirm it fails without the emitter. +- **A `patch_ib` member that immersed-boundary ghost-point code reads must also be set in `s_add_cloud_particle`** (`src/simulation/m_particle_cloud.fpp`). `particle_cloud_ibs` is allocated without default initialization, and `s_reduce_ib_patch_array` copies the whole struct into `patch_ib`, overwriting the defaults assigned in `s_assign_default_values_to_user_inputs`. Anything left unset reaches the solver as uninitialized memory, and only where the allocation is not already zero-filled. A platform-only NaN is the signature of this class: a garbage `v_blow` once failed an AMD lane with `ICFL is NaN` while every NVIDIA lane and all local runs passed. +- **Runtime checks go where they run.** Shared constraints belong in `src/common/m_checker_common.fpp`, simulation-only ones in `src/simulation/m_checker.fpp`, and pre- and post-process ones in their own `m_checker.fpp`. Those two `s_check_inputs` are currently empty; that is still the correct home for their checks, not `m_checker_common`. +- **Analytic initial conditions are compiled into the binary** and their expressions are AST-validated at case load, so syntax errors and unknown variables surface immediately and by name. Each IC variable maps to an `eqn_idx` expression in `QPVF_IDX_VARS` (`toolchain/mfc/case.py`); adding a patch-settable conserved variable means updating that map and the Fortran `eqn_idx` builder together, because a mismatch is a silent wrong index. +- **Under `--case-optimization` the baked-in constants are dropped from the namelist**, so changing one requires a rebuild rather than a case-file edit. + ### Compiler Portability - Any compiler-specific code (`#ifdef __INTEL_COMPILER` etc.) must have fallbacks for all four supported compilers. diff --git a/docs/documentation/gpuParallelization.md b/docs/documentation/gpuParallelization.md index dbab32ef27..8b205715de 100644 --- a/docs/documentation/gpuParallelization.md +++ b/docs/documentation/gpuParallelization.md @@ -864,6 +864,55 @@ while the host still registers it. The first launch aborts with followed by a segmentation fault. Never place a GPU kernel inside a `block` construct; hoist it into its own (module) subroutine with the locals passed as arguments. +## Silent-Failure Traps + +Every entry here was measured. They share a failure mode: the build stays green and the +answer is wrong, or one backend diverges from all the others. + +- **Do not wrap `GPU_LOOP` in `GPU_PARALLEL` for spatial loops.** `GPU_LOOP` emits empty + directives on Cray and AMD, so the loop runs serially with no diagnostic. Spatial loops + always use `GPU_PARALLEL_LOOP` / `END_GPU_PARALLEL_LOOP`. +- **An array whose bound is a device global** (`dimension(num_fluids)`, + `dimension(num_species)`) may be passed to a device routine from a parallel-loop body, + but **not from inside another `GPU_ROUTINE(parallelism='[seq]')`**. Cray OpenACC rejects + the second form with `ftn-7066 ... Global in accelerator routine without declare`, and + reports it at whatever line it gave up on: remove one trigger and the message walks + forward to the next call, so the reported line is not the cause. Only the plain lanes + fail, since `--case-optimization` turns those bounds into `parameter`s — a green case-opt + lane beside a failing plain one is the signature. Form such a call in the loop body and + pass scalars deeper. Neither `cray_inline`, nor a `num_fluids_max` bound, nor dropping + optional dummies avoids it; all three were tried. +- **A device routine containing any `GPU_LOOP` must be called with scalars, never with an + array element.** On Cray OpenACC 19.0.0 through 21.0.2 at `-O2` (`-O0` and `-O1` are + correct, OpenMP offload is unaffected) the element is misaddressed: an `intent(in)` + element reads as garbage and an `intent(out)` element is never written. Every `routine` + level is affected, including a conforming `loop vector` inside `routine vector`. Either + ingredient alone is fine, which is why a call like `s_compute_pressure(q%%sf(j,k,l), ...)` + into a loop-free helper works. Copy elements into locals before the call and receive into + a local. Do not instead delete the `seq` directives: they are the idiom every device + routine here uses. See [#1815](https://github.com/MFlowCode/MFC/issues/1815). +- **Call `m_thermochem` species routines from the kernel, not from inside a + `GPU_ROUTINE`.** Calling `get_species_*` from within a device routine gives Cray OpenMP a + runtime `Memory access fault by GPU node-N` on the first step while every other backend + runs. The build is clean and only a case that reaches the path shows it. Evaluate them at + the call site and pass the arrays in. +- **nvfortran 23.11 and 24.1 segfault** (`fort2 TERMINATED by signal 11`) on a caller that + passes a `parameter` array from `m_thermochem`, such as `molecular_weights`, into a + declare-target routine. Read such arrays directly in the kernel, or pass a plain local + computed from them. +- **The `USING_AMD` fypp guards are load-bearing, not a stale workaround.** They swap a + device-global array bound for a literal in `src/common/include/shared_parallel_macros.fpp` + and its 86 use sites. Setting `USING_AMD = False` and rebuilding amdflang `--gpu mp` + without case optimization compiles completely clean, then produces NaNs in CBC, the + `wave_speeds=2` Riemann path, immersed boundaries, surface tension, QBMM and viscous + cases, and MHD HLLD, while both Lagrange bubble cases complete with out-of-tolerance + answers. A compile-only check returns green, so any attempt to remove these must run the + tests rather than just build. +- `@:ACC_SETUP_VFs` and `@:ACC_SETUP_SFs` compile only under Cray. Around MPI, use + `GPU_UPDATE(host=...)` before a send and `GPU_UPDATE(device=...)` after a receive. + +------------------------------------------------------------------------------------------ + ## Compiler Documentation - [Cray & OpenMP Docs](https://cpe.ext.hpe.com/docs/24.11/cce/man7/intro_openmp.7.html#environment-variables) diff --git a/docs/documentation/testing.md b/docs/documentation/testing.md index 08b1b771e2..be50883f78 100644 --- a/docs/documentation/testing.md +++ b/docs/documentation/testing.md @@ -13,7 +13,7 @@ A test is considered passing when our error tolerances are met in order to maint `./mfc.sh test` has the following unique options: - `-l` outputs the full list of tests - `--from` (`-f)` and `--to` (`t`) restrict testing to a range of contiguous slugs -- `--only` (`-o`) restricts testing to a non-contiguous range of tests based on if their trace contains a certain feature +- `--only` (`-o`) restricts testing to a non-contiguous range of tests whose trace contains a given whole trace element (see [Selection and Execution Pitfalls](#selection-and-execution-pitfalls) for the exact matching rules) - `--test-all` (`a`) test post process and ensure the Silo database files are correct - `--percent` (`%`) to specify a percentage of the test suite to select at random and test - `--max-attempts` (`-m`) the maximum number of attempts to make on a test before considering it failed @@ -92,6 +92,38 @@ If a trace is empty (that is, the empty string `""`), it will not appear in the Finally, the case is appended to the `cases` list, which will be returned by the `list_cases` function. +### Selection and Execution Pitfalls + +Each of these fails quietly rather than loudly. + +- **`--only` matches whole trace elements, not substrings**, and it ANDs labels while ORing + UUIDs (`_filter_only` in `toolchain/mfc/test/test.py`). `--only bubbles` matches nothing, + because the trace element is `Bubbles`; `--only low_Mach=1 low_Mach=2` asks for cases + carrying both labels at once and also matches nothing. An empty selection then exits + **143**, which reads like an external kill rather than an empty filter. Pass UUIDs when + you want the union of several groups. +- **Sibling `define_case_d` calls at the same stack level are never combined.** Two switches + that only matter together therefore get no effective coverage unless one is pushed onto + the stack and the other defined beneath it — `avg_state=1`, for instance, is only read + when `wave_speeds=2`. Check reachability before trusting that a flag is tested. +- **`--no-build` silently runs whatever binary is already on disk**, including one built for + a different configuration. Chemistry has its own configuration that a plain `./mfc.sh + build` never produces, so a `--no-build` run can report failures from stale binaries and + hide real compile breaks. Run chemistry-touching sets without it. +- **Identify the newest binary by the binary's own mtime**, not by its install directory's: + a stale configuration's directory can be newer than a fresh build's. +- **The pre-commit hook lives in the main repository's `.git/hooks/`**, and git exports + `GIT_DIR` there during a commit, so from a worktree the toolchain lint enumerates the + other checkout and fails. Run `./mfc.sh precheck` by hand and commit with `--no-verify`. +- **`/tmp` is node-local.** Scratch does not survive a compute-node change, and its absence + is silence rather than an error. Keep patches and resource baselines on a shared + filesystem. +- **An unexplained golden-file difference is a bug report, not noise to be regenerated + away.** Regenerate only the affected tests. + +Tests are generated programmatically in `toolchain/mfc/test/cases.py`; a test's UUID is the +CRC32 of its trace string, and `./mfc.sh test -l` lists every one. + ### Testing Post Process To test the post-processing code, append the `-a` or `--test-all` option: