Fix bug with num_fluids = 3 and AMD

[anandrdbz]

Description

Fixing a bug when num_fluids = 3 and MFC_CASE_OPTIMIZATION is off with AMD compilers.

There is a check guarding this when num_fluids > 3 with these conditions, that can be resolved once the internal issue is fixed with private arrays

Closes #1368

Type of change

• Bug fix
• New feature
• Refactor
• Documentation
• Other: describe

Testing

How did you test your changes?

Checklist

• I added or updated tests for new behavior
• I updated documentation if user-facing behavior changed

See the developer guide for full coding standards.

GPU changes (expand if you modified src/simulation/)

• GPU results match CPU results
• Tested on NVIDIA GPU or AMD GPU

AI code reviews

Reviews are not triggered automatically. To request a review, comment on the PR:

• @coderabbitai review — incremental review (new changes only)
• @coderabbitai full review — full review from scratch
• /review — Qodo review
• /improve — Qodo code suggestions
• @claude full review — Claude full review (also triggers on PR open/reopen/ready)
• Add label claude-full-review — Claude full review via label

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0)

1. Uninitialized alpha element read 🐞 Bug ☼ Reliability

Description

In s_correct_internal_energies, the AMD/non-case-optimized branch declares alpha/alpha_rho with
dimension(3) but only initializes indices 1:num_fluids; when num_fluids is 1 or 2 and mpp_lim is
enabled, the whole-array normalization alpha = alpha/max(sum_alpha, sgm_eps) reads alpha(3)
uninitialized. This is undefined behavior and can surface as NaNs or FP exceptions in
debug/IEEE-trapping builds.

Code

src/simulation/m_pressure_relaxation.fpp[R219-220]

       #:if not MFC_CASE_OPTIMIZATION and USING_AMD
-            real(wp), dimension(2) :: alpha_rho, alpha
+            real(wp), dimension(3) :: alpha_rho, alpha

Evidence

The PR changes the AMD path to dimension(3); later in the same subroutine only alpha(i) for
i=1..num_fluids is set, but a whole-array assignment normalizes all 3 elements, which requires
reading alpha(3) even when num_fluids<3. The input checker on AMD (when case optimization is off)
only prohibits num_fluids>3 (so 1 or 2 are valid), and in non-case-optimized builds num_fluids is a
runtime variable, making the num_fluids<3 scenario reachable.

src/simulation/m_pressure_relaxation.fpp[213-289]
src/common/m_checker_common.fpp[51-58]
src/simulation/m_global_parameters.fpp[119-130]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
On the `#:if not MFC_CASE_OPTIMIZATION and USING_AMD` branch, `alpha`/`alpha_rho` are sized to 3, but only `1:num_fluids` are initialized. When `mpp_lim` is enabled, `alpha = alpha/max(sum_alpha, sgm_eps)` performs a whole-array assignment that reads the uninitialized tail (e.g., `alpha(3)` when `num_fluids=2`).
### Issue Context
AMD + non-case-optimized builds allow `num_fluids` to be 1..3. After this PR’s 2→3 sizing change, `num_fluids=2` becomes a new case where the tail element is not initialized but is still read during normalization.
### Fix Focus Areas
- src/simulation/m_pressure_relaxation.fpp[219-269]
### Suggested change
- Initialize arrays before filling (e.g., `alpha = 0._wp; alpha_rho = 0._wp`).
- Replace whole-array normalization with a slice:
- `alpha(1:num_fluids) = alpha(1:num_fluids) / max(sum_alpha, sgm_eps)`
This keeps the AMD fixed-size workaround while avoiding any reads of uninitialized elements.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 68df298

Results up to commit 68df298

🐞 Bugs (1) 📘 Rule violations (0)

1. Uninitialized alpha element read 🐞 Bug ☼ Reliability

Description

In s_correct_internal_energies, the AMD/non-case-optimized branch declares alpha/alpha_rho with
dimension(3) but only initializes indices 1:num_fluids; when num_fluids is 1 or 2 and mpp_lim is
enabled, the whole-array normalization alpha = alpha/max(sum_alpha, sgm_eps) reads alpha(3)
uninitialized. This is undefined behavior and can surface as NaNs or FP exceptions in
debug/IEEE-trapping builds.

Code

src/simulation/m_pressure_relaxation.fpp[R219-220]

        #:if not MFC_CASE_OPTIMIZATION and USING_AMD
-            real(wp), dimension(2) :: alpha_rho, alpha
+            real(wp), dimension(3) :: alpha_rho, alpha

Evidence

The PR changes the AMD path to dimension(3); later in the same subroutine only alpha(i) for
i=1..num_fluids is set, but a whole-array assignment normalizes all 3 elements, which requires
reading alpha(3) even when num_fluids<3. The input checker on AMD (when case optimization is off)
only prohibits num_fluids>3 (so 1 or 2 are valid), and in non-case-optimized builds num_fluids is a
runtime variable, making the num_fluids<3 scenario reachable.

src/simulation/m_pressure_relaxation.fpp[213-289]
src/common/m_checker_common.fpp[51-58]
src/simulation/m_global_parameters.fpp[119-130]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
On the `#:if not MFC_CASE_OPTIMIZATION and USING_AMD` branch, `alpha`/`alpha_rho` are sized to 3, but only `1:num_fluids` are initialized. When `mpp_lim` is enabled, `alpha = alpha/max(sum_alpha, sgm_eps)` performs a whole-array assignment that reads the uninitialized tail (e.g., `alpha(3)` when `num_fluids=2`).

### Issue Context
AMD + non-case-optimized builds allow `num_fluids` to be 1..3. After this PR’s 2→3 sizing change, `num_fluids=2` becomes a new case where the tail element is not initialized but is still read during normalization.

### Fix Focus Areas
- src/simulation/m_pressure_relaxation.fpp[219-269]

### Suggested change
- Initialize arrays before filling (e.g., `alpha = 0._wp; alpha_rho = 0._wp`).
- Replace whole-array normalization with a slice:
 - `alpha(1:num_fluids) = alpha(1:num_fluids) / max(sum_alpha, sgm_eps)`
This keeps the AMD fixed-size workaround while avoiding any reads of uninitialized elements.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request updates the src/simulation/m_pressure_relaxation.fpp file, specifically modifying the s_correct_internal_energies subroutine. Within the #:if not MFC_CASE_OPTIMIZATION and USING_AMD compilation branch, the dimensions of two local work arrays—alpha_rho and alpha—are increased from real(wp), dimension(2) to real(wp), dimension(3). The core algorithmic logic and control flow remain unchanged, and no public interfaces are affected.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	PR description identifies the bug and issue number but lacks testing details, GPU validation confirmation, and checklist item completion.	Fill in the 'Testing' section with specific test cases or reproduction steps. Mark relevant GPU checklist items to confirm testing on AMD GPU and CPU result validation.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing a bug specific to num_fluids = 3 on AMD compilation branch, which matches the core change in the code.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[qodo-code-review]

Persistent review updated to latest commit 68df298

[github-actions]

Claude Code Review

Head SHA: 68df298

Files changed:

• 1
• src/simulation/m_pressure_relaxation.fpp

Findings

Hardcoded array dimension still insufficient for num_fluids > 3 on AMD non-case-optimization builds

src/simulation/m_pressure_relaxation.fpp, s_correct_internal_energies:

#:if not MFC_CASE_OPTIMIZATION and USING_AMD
    real(wp), dimension(3) :: alpha_rho, alpha   ! changed from 2 -> 3
#:else
    real(wp), dimension(num_fluids) :: alpha_rho, alpha
#:endif

The fix correctly resolves an out-of-bounds write when num_fluids = 3 (the loops do i = 1, num_fluids over alpha_rho(i) and alpha(i) now have a large enough backing array). However, the same out-of-bounds hazard persists for any case where num_fluids > 3 in an AMD non-case-optimization build. The loops in s_correct_internal_energies iterate up to num_fluids unconditionally, so a 4-fluid simulation on AMD would immediately write past the end of both arrays.

s_equilibrate_pressure has the same dimension(3) pattern for pres_K_init / rho_K_s, so this is now consistent — but both routines share the latent limitation. If AMD builds are expected to support num_fluids > 3, the bound should be made safe (e.g. a named constant reflecting the true maximum). If num_fluids <= 3 is a hard constraint for AMD non-case-optimization builds, that constraint should be enforced at startup via @:PROHIBIT.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.75%. Comparing base (36fc04c) to head (68df298).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1394   +/-   ##
=======================================
  Coverage   64.75%   64.75%           
=======================================
  Files          71       71           
  Lines       18721    18721           
  Branches     1551     1551           
=======================================
  Hits        12123    12123           
  Misses       5640     5640           
  Partials      958      958           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[anandrdbz]

I think there's something wrong with phoenix runners, the jobs fail instantly

[sbryngelson]

Claude Code Review

PR #1394 — Fix bug with num_fluids = 3 and AMD

One-line fix: dimension(2) → dimension(3) for alpha_rho and alpha in the #:if not MFC_CASE_OPTIMIZATION and USING_AMD branch of s_correct_internal_energies.

---

Strengths

• Fix is correct and safe. src/common/m_checker_common.fpp enforces @:PROHIBIT(num_fluids > 3, ...) inside s_check_amd, so dimension(3) is the exact tight upper bound for this code path.
• Consistent with existing pattern. The parallel AMD branch in s_equilibrate_pressure already uses dimension(3) for its local temporaries. This PR makes s_correct_internal_energies consistent.
• No precision violations. Local temporaries correctly use wp, GPU macros used correctly.

---

Important Issues

1. Whole-array normalization reads uninitialized elements when num_fluids < 3 (pre-existing, but PR extends exposure)

src/simulation/m_pressure_relaxation.fpp, line ~252:

alpha = alpha/max(sum_alpha, sgm_eps)

This whole-array statement operates on all 3 elements. Only indices 1..num_fluids are initialized in the load loop. When num_fluids = 1 or 2, elements alpha(num_fluids+1..3) are uninitialized at this point. The uninitialized elements are never consumed downstream (all loops use do i = 1, num_fluids), so correctness is maintained in practice. However, reading an uninitialized real(wp) is technically undefined behavior in Fortran and may trigger floating-point exception traps on AMD GPU hardware or in debug builds with sNaN detection.

This latent issue existed before this PR for num_fluids = 1. The minimal fix:

do i = 1, num_fluids
    alpha(i) = alpha(i)/max(sum_alpha, sgm_eps)
end do

2. No regression test added for the fixed configuration

The PR checklist testing section is empty and CI was not run. There is no test covering num_fluids = 3 + AMD compiler + MFC_CASE_OPTIMIZATION off — exactly the configuration this bug affected.

---

Suggestions

• Add a brief comment above the dimension(3) declarations cross-referencing the s_check_amd guard, so future maintainers understand why the hardcoded bound is safe.
• Line ~215: q_cons_vf(eqn_idx%E + i) is arithmetically correct but using q_cons_vf(eqn_idx%adv%beg + i - 1) explicitly would be more self-documenting and consistent with other volume-fraction accesses in this file.