Copy the ib flag to the device - #1936
sbryngelson wants to merge 1 commit into
Conversation
#1792 added two device-side reads of ib -- skipping immersed-boundary interior cells in the adaptive-dt reduction (s_compute_dt) and in the CFL stability statistics (s_write_run_time_information). ib is GPU_DECLARE'd through SIM_GPU_DECL_VARS but was never GPU_UPDATE'd, so on the device it stayed .false. and GPU builds kept including IB interior cells in dt while CPU builds excluded them. With cfl_adap_dt=T this gives GPU runs a different dt from CPU runs.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The CPU/GPU synchronization fix lacks an automated regression test.
Review effort: Balanced
Findings: 1
What changed in this PR
Synchronizes the immersed-boundary flag to GPU devices so adaptive timestep and CFL calculations match CPU behavior.
Changes:
- Adds
ibto global GPU parameter synchronization.
| File | Description |
|---|---|
src/simulation/m_global_parameters.fpp |
Copies ib to device storage during initialization. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| $:GPU_UPDATE(device='[alt_soundspeed, acoustic_source, num_source]') | ||
| $:GPU_UPDATE(device='[dt, sys_size, buff_size, eqn_idx, mpp_lim, bubbles_euler, hypoelasticity, alt_soundspeed, & | ||
| & avg_state, model_eqns, mixture_err, grid_geometry, cyl_coord, mp_weno, weno_eps, teno_CT, low_Mach]') | ||
| & avg_state, model_eqns, mixture_err, grid_geometry, cyl_coord, mp_weno, weno_eps, teno_CT, low_Mach, ib]') |
|
Superseded by #1937 (same commit, opened from a fork per the repo's contribution rules). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1936 +/- ##
==========================================
- Coverage 62.82% 62.80% -0.02%
==========================================
Files 86 86
Lines 22394 22385 -9
Branches 3305 3304 -1
==========================================
- Hits 14070 14060 -10
- Misses 6071 6073 +2
+ Partials 2253 2252 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
ibgets a device copy (GPU_DECLARE(create=...)viaSIM_GPU_DECL_VARS), but nothing ever copies its value to the GPU, so inside device kernels it stays.false.. #1792 added two device-side reads of it:s_compute_dt(m_time_steppers.fpp): skip IB-interior cells in the adaptive-dt reductions_write_run_time_information(m_data_output.fpp): skip IB-interior cells in the CFL stability statisticsOn GPU builds both checks are silently skipped. GPUs keep including IB-interior cells when choosing dt, while CPU builds exclude them. Any case with
ibandcfl_adap_dt = Tthen runs with a different dt on GPU than on CPU. This addsibto the existingGPU_UPDATE(device=...)ins_initialize_global_parameters_module.How it showed up
In #1821, the
2D -> Example -> ibm_reacting_surfacetest (F52F0D4C) gives the same wrong numbers on every GPU build (Frontier CCE-acc, CCE-omp, AMD-omp, and Phoenix NVHPC-acc: 0.65037 vs. golden 0.6487849), while every CPU build (GNU, CCE, AMD) matches. Before the merge of #1792 into that branch, CPU and GPU agreed. After it, GPU stayed on the old value and only CPU moved. No master test currently triggers the bug.The other device-declared parameters that are never updated (
down_sample,igr_pres_lim,recon_type,weno_order) are case-optimization parameters that are read on the host or baked in at compile time, so this PR leaves them alone.This PR was prepared with Claude Code (diagnosis from the #1821 CI logs and the fix).
Acknowledgement