|
1 | | -# GitHub Copilot Instructions – Pull-Request Reviews for MFC |
| 1 | +# AI Code Review Instructions for MFC |
2 | 2 |
|
3 | | -These instructions guide **GitHub Copilot Code Review** and **Copilot Chat** when they evaluate pull requests in this repository. |
| 3 | +These instructions guide AI code reviewers (GitHub Copilot, CodeRabbit, Qodo, Cubic, CodeAnt, etc.) when evaluating pull requests in this repository. |
4 | 4 |
|
5 | | ---- |
6 | | - |
7 | | -## 1 Project Context (always include) |
| 5 | +**Coding standards, common pitfalls, and contribution guidelines:** `docs/documentation/contributing.md` |
| 6 | +**GPU macro API reference:** `docs/documentation/gpuParallelization.md` |
8 | 7 |
|
9 | | -* **Project:** MFC (exascale many-physics solver) written in **modern Fortran 2008+**, generated with **Fypp**. |
10 | | -* **Directory layout:** |
11 | | - * Sources in `src/`, tests in `tests/`, examples in `examples/`. |
12 | | - * Most source files are templated `.fpp`; CMake transpiles them to `.f90`. |
13 | | -* **Fypp macros** are in `src/<subprogram>/include/`, where `<subprogram>` is `simulation`, `common`, `pre_process`, or `post_process`. Review these first. |
14 | | -* Only `simulation` (plus its `common` dependencies) is GPU-accelerated with **OpenACC**. |
15 | | - |
16 | | -> **Copilot, when reviewing:** |
17 | | -> * Treat the codebase as free-form Fortran 2008+ with `implicit none`, explicit `intent`, and standard intrinsics. |
18 | | -> * Prefer `module … contains … subroutine foo()` over legacy constructs; flag uses of `COMMON`, file-level `include`, or other global state. |
| 8 | +Formatting and linting are enforced by pre-commit hooks. Focus review effort on correctness, not style. |
19 | 9 |
|
20 | 10 | --- |
21 | 11 |
|
22 | | -## 2 Incremental-Change Workflow |
| 12 | +## 1 Project Context |
23 | 13 |
|
24 | | -Copilot, when reviewing: |
25 | | -* Encourage small, buildable commits |
| 14 | +* **Project:** MFC (exascale many-physics solver) written in **modern Fortran 2008+**, preprocessed with **Fypp**. |
| 15 | +* **Directory layout:** |
| 16 | + * Sources in `src/`, tests in `tests/`, examples in `examples/`, Python toolchain in `toolchain/`. |
| 17 | + * Most source files are `.fpp` (Fypp templates); CMake transpiles them to `.f90`. |
| 18 | +* **Fypp macros** are in `src/<subprogram>/include/`, where `<subprogram>` is `simulation`, `common`, `pre_process`, or `post_process`. |
| 19 | +* Only `simulation` (plus its `common` dependencies) is GPU-accelerated via **OpenACC**. |
| 20 | +* Code must compile with **GNU gfortran**, **NVIDIA nvfortran**, **Cray ftn**, and **Intel ifx**. |
| 21 | +* Precision modes: double (default), single, and mixed (`wp` = working precision, `stp` = storage precision). |
| 22 | +* **Python toolchain** requires **Python 3.10+** — do not suggest `from __future__` imports or other backwards-compatibility shims. |
26 | 23 |
|
27 | 24 | --- |
28 | 25 |
|
29 | | -## 3 Style & Naming Conventions (*.fpp / *.f90) |
| 26 | +## 2 What to Review |
30 | 27 |
|
31 | | -| Element | Rule | |
32 | | -|---------|------| |
33 | | -| Indentation | 2 spaces; continuation lines align beneath &. | |
34 | | -| Case | Lower-case keywords & intrinsics (do, end subroutine, …). | |
35 | | -| Modules | m_<feature> (e.g. m_transport). | |
36 | | -| Public subroutines | s_<verb>_<noun> (s_compute_flux). | |
37 | | -| Public functions | f_<verb>_<noun>. | |
38 | | -| Routine size | subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, file ≤ 1000. | |
39 | | -| Arguments | ≤ 6; else use a derived-type params struct. | |
40 | | -| Forbidden | goto (except legacy), COMMON, save globals. | |
41 | | -| Variables | Every arg has explicit intent; use dimension/allocatable/pointer as appropriate. | |
42 | | -| Errors | Call s_mpi_abort(<msg>), never stop or error stop. | |
| 28 | +See the **Common Pitfalls** section of `docs/documentation/contributing.md` for the full reference. Key review priorities: |
43 | 29 |
|
44 | | -Copilot, when reviewing: |
45 | | -* Flag violations of any cell above. |
46 | | -* Suggest refactors when size or argument limits are exceeded. |
47 | | -* Ensure private helpers stay inside their defining module and avoid nested procedures. |
| 30 | +1. **Correctness over style** — logic bugs, numerical issues, array bounds (non-unity lower bounds with ghost cells) |
| 31 | +2. **Precision mixing** — `stp` vs `wp` conversions, no double-precision intrinsics (`dsqrt`, `dble`, `real(8)`, etc.) |
| 32 | +3. **Memory** — `@:ALLOCATE`/`@:DEALLOCATE` pairing, GPU pointer setup (`@:ACC_SETUP_VFs`/`@:ACC_SETUP_SFs`) |
| 33 | +4. **MPI** — halo exchange pack/unpack offsets, `GPU_UPDATE` around MPI calls, buffer sizing |
| 34 | +5. **GPU** — no raw OpenACC/OpenMP pragmas (use Fypp GPU macros), `private(...)` on loop-local variables, no `stop`/`error stop` in device code |
| 35 | +6. **Physics** — pressure formula must match `model_eqns`, `case_validator.py` constraints for new parameters |
| 36 | +7. **Compiler portability** — all four compilers, Fypp macros for GPU and CPU builds |
| 37 | +8. **`src/common/` blast radius** — changes affect all three executables |
48 | 38 |
|
49 | 39 | --- |
50 | 40 |
|
51 | | -## 4 OpenACC Guidelines (for GPU kernels) |
52 | | - |
53 | | -Wrap tight loops: |
54 | | - |
55 | | -```fortran |
56 | | -!$acc parallel loop gang vector default(present) reduction(...) |
57 | | -``` |
58 | | - |
59 | | -* Add collapse(n) when safe. |
60 | | -* Declare loop-local variables with private(...). |
61 | | -* Allocate large arrays with managed or move them into a persistent !$acc enter data region at start-up. |
62 | | -* Avoid stop/error stop inside device code. |
63 | | -* Code must compile with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort. |
64 | | - |
65 | | ---- |
| 41 | +## 3 PR-Pattern Triggers |
66 | 42 |
|
67 | | -## 5 Review Checklist (what Copilot should verify) |
| 43 | +Flag these patterns when reviewing a pull request: |
68 | 44 |
|
69 | | -1. Buildability: PR includes build instructions or CI passes the staged build. |
70 | | -2. Tests: Focused tests are added/updated. |
71 | | -3. Style compliance: All rules in §3 are satisfied. |
72 | | -4. Module hygiene: No new global state; proper namespacing. |
73 | | -5. Performance: GPU code follows §4; no large host/device transfers in hot loops. |
74 | | -6. Documentation: Updated in-code comments and, when needed, README or docs site. |
75 | | -7. Regressions: No changes to outputs of golden tests without clear justification. |
| 45 | +* PR adds a parameter in `toolchain/mfc/params/definitions.py` but does not update the `namelist /user_inputs/` in `src/*/m_start_up.fpp` or declare it in `src/*/m_global_parameters.fpp` |
| 46 | +* PR adds a parameter with cross-parameter constraints but does not add validation in `toolchain/mfc/case_validator.py` |
| 47 | +* PR modifies files in `src/common/` but does not mention testing all three targets (pre_process, simulation, post_process) |
| 48 | +* PR adds `@:ALLOCATE` calls without matching `@:DEALLOCATE` in the corresponding finalization subroutine |
| 49 | +* PR renames or moves files referenced in `docs/documentation/contributing.md` or `docs/documentation/gpuParallelization.md` without updating those docs |
| 50 | +* PR adds a new `.fpp` module missing `implicit none` or missing `private`/`public` declarations |
| 51 | +* PR modifies MPI pack/unpack logic in one sweep direction without updating all directions |
| 52 | +* PR adds a new physics feature or model without a corresponding regression test |
0 commit comments