diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index a6913c6d0..7547db8b0 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -1,8 +1,8 @@ * 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. + * 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 + * that change CFD results need verification that the PR is correct * follow template * that break a feature but promise a followup PR to fix it are rejected * Commands: @@ -11,5 +11,5 @@ * 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 + * GPU macros should follow the source's existing GPU macro principles and patterns + * Functions/subroutines/modules shorter is better while being correct, fast, and separating concerns diff --git a/.github/scripts/preflight.sh b/.github/scripts/preflight.sh index ba0244f37..ec07f7d52 100755 --- a/.github/scripts/preflight.sh +++ b/.github/scripts/preflight.sh @@ -57,8 +57,10 @@ fi # microarchitecture dies with SIGILL, which would be reported as a bad node and # get a perfectly healthy one excluded. newest_syscheck() { - find "$@" -name syscheck -type f -printf '%T@ %p\n' 2>/dev/null \ - | sort -rn | head -1 | cut -d' ' -f2- + # ls -t rather than find -printf: -printf is GNU-only, and on a BSD find it + # fails into 2>/dev/null, so discovery silently returns nothing and every + # probe is skipped as "no syscheck binary". + find "$@" -name syscheck -type f -exec ls -t {} + 2>/dev/null | head -1 } syscheck_bin=$(newest_syscheck build/install -path "*${device}*") @@ -132,7 +134,9 @@ run_probe() { fi } -run_probe "${launcher[@]}" +# ${arr[@]+"${arr[@]}"} rather than "${arr[@]}": under set -u, bash 3.2 (which is +# what macOS ships) treats an empty array expansion as an unbound variable. +run_probe ${launcher[@]+"${launcher[@]}"} # If this launcher does not take the flags we added, drop them and probe again # rather than reporting a verdict about the node. Otherwise a launcher that diff --git a/.github/workflows/homebrew-release.yml b/.github/workflows/homebrew-release.yml index fe616e96c..7cb31b12f 100644 --- a/.github/workflows/homebrew-release.yml +++ b/.github/workflows/homebrew-release.yml @@ -1,10 +1,12 @@ name: Update Homebrew Formula on Release -# Triggers when a new version tag is pushed +# Triggers when a release is published. Not `push: tags`, which GitHub skips +# when the tagged commit message carries [skip ci] -- that silently dropped the +# v5.6.0 and v5.7.0 formula updates, while docker.yml fired for both because a +# release event is not a push event. on: - push: - tags: - - 'v*' + release: + types: [published] pull_request: branches: [master] paths: @@ -28,6 +30,9 @@ permissions: jobs: update-homebrew-tap: name: Update homebrew-mfc tap + # A prerelease tag (v5.8.0-rc1) would fail the X.Y.Z check below, so skip it + # rather than reporting a red release job. + if: ${{ github.event_name != 'release' || !github.event.release.prerelease }} runs-on: ubuntu-latest environment: name: homebrew @@ -44,8 +49,9 @@ jobs: VERSION="5.2.0" echo "::notice::PR test mode - using version $VERSION" else - # Extract version from tag (remove 'v' prefix) - VERSION="${GITHUB_REF#refs/tags/v}" + # Extract version from the released tag (remove 'v' prefix) + VERSION="${{ github.event.release.tag_name }}" + VERSION="${VERSION#v}" fi if [[ ! "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then @@ -97,7 +103,7 @@ jobs: echo "- Update formula" >> $GITHUB_STEP_SUMMARY echo "- Push to tap" >> $GITHUB_STEP_SUMMARY echo "" >> $GITHUB_STEP_SUMMARY - echo "The full workflow will run when a \`v*\` tag is pushed after merge." >> $GITHUB_STEP_SUMMARY + echo "The full workflow will run when a release is published after merge." >> $GITHUB_STEP_SUMMARY - name: Checkout homebrew-mfc tap if: ${{ github.event_name != 'pull_request' }} diff --git a/docs/documentation/contributing.md b/docs/documentation/contributing.md index d42dc09e8..dd3c0f33c 100644 --- a/docs/documentation/contributing.md +++ b/docs/documentation/contributing.md @@ -217,7 +217,7 @@ Both human reviewers and AI code reviewers reference this section. ### 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. +- **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. diff --git a/docs/documentation/gpuParallelization.md b/docs/documentation/gpuParallelization.md index 8b205715d..85b0d7719 100644 --- a/docs/documentation/gpuParallelization.md +++ b/docs/documentation/gpuParallelization.md @@ -864,7 +864,7 @@ 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 +## Silent-Failure Traps {#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. @@ -874,7 +874,7 @@ answer is wrong, or one backend diverges from all the others. 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 + 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 diff --git a/docs/documentation/testing.md b/docs/documentation/testing.md index be50883f7..b23e15231 100644 --- a/docs/documentation/testing.md +++ b/docs/documentation/testing.md @@ -92,7 +92,7 @@ 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 +### Selection and Execution Pitfalls {#selection-and-execution-pitfalls} Each of these fails quietly rather than loudly. diff --git a/toolchain/mfc/lint_source.py b/toolchain/mfc/lint_source.py index 7dca078f0..4093fcf7d 100644 --- a/toolchain/mfc/lint_source.py +++ b/toolchain/mfc/lint_source.py @@ -629,8 +629,10 @@ def check_device_routine_element_args(repo_root: Path) -> list[str]: routine containing any `GPU_LOOP`, called with an array element as an actual argument, reads 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 - sbryngelson/compiler-bugs cce/acc-routine-element-by-reference. + to a scalar before the call and receive results into a scalar. See the Silent-Failure + Traps section of docs/documentation/gpuParallelization.md, MFC issue + https://github.com/MFlowCode/MFC/issues/1815, and the reproducer at + https://github.com/sbryngelson/compiler-bugs/tree/main/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 +685,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 sbryngelson/compiler-bugs cce/acc-routine-element-by-reference") + errors.append(f" {rel}:{line_no} `{arg}` into `{name}` (a device routine with a seq loop): pass a scalar, see docs/documentation/gpuParallelization.md (Silent-Failure Traps)") return errors diff --git a/toolchain/mfc/test_preflight.py b/toolchain/mfc/test_preflight.py index 9125fb667..f94e9e76e 100644 --- a/toolchain/mfc/test_preflight.py +++ b/toolchain/mfc/test_preflight.py @@ -44,7 +44,7 @@ def workspace(tmp_path): # stand in for a launcher the test meant to be absent. sysbin = tmp_path / "sysbin" sysbin.mkdir() - for tool in ("bash", "find", "head", "tail", "cat", "sed", "grep", "tr", "cut", "date", "mkdir", "mv", "rm", "hostname", "env", "sort", "wc", "dirname", "basename"): + for tool in ("bash", "find", "head", "tail", "cat", "sed", "grep", "tr", "cut", "date", "mkdir", "mv", "rm", "hostname", "env", "sort", "wc", "dirname", "basename", "ls"): for root in ("/usr/bin", "/bin"): src = Path(root) / tool if src.exists():