Fix missing private clauses in 3D viscous GPU loops

[sbryngelson]

User description

Summary

• The 3D shear stress and bulk stress GPU_PARALLEL_LOOP directives were missing rho_visc, gamma_visc, pi_inf_visc, and alpha_visc_sum from their private clauses
• The corresponding 2D loops already included these variables
• Without privatization, these variables can cause race conditions when running on GPU

Test plan

• Verify 3D viscous test cases produce correct results on GPU
• No golden file changes expected (fixes GPU race condition, CPU results unchanged)

🤖 Generated with Claude Code

---

CodeAnt-AI Description

Fix GPU data races in 3D viscous loops by privatizing loop indices and temporaries

What Changed

• The 3D shear- and bulk-stress GPU parallel regions now privately scope the loop indices (i, j, k, l, q) and per-thread viscous temporaries (rho_visc, gamma_visc, pi_inf_visc, alpha_visc_sum and related temporaries), preventing those variables from being shared across GPU threads
• Privatization was added consistently across all affected 3D viscous loops so GPU runs do not suffer from cross-thread interference
• CPU execution and numerical results remain unchanged

Impact

✅ Correct 3D viscous results on GPU
✅ Fewer GPU race-condition artifacts in viscous simulations
✅ No change to CPU outputs

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

[cubic-dev-ai]

No issues found across 1 file

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

[Copilot]

Pull request overview

Fixes a GPU data-race hazard in the 3D viscous stress kernels by aligning their GPU_PARALLEL_LOOP privatization with the already-correct 2D implementations.

Changes:

• Add missing mixture variables (rho_visc, gamma_visc, pi_inf_visc, alpha_visc_sum) to the private clauses for the 3D shear-stress and bulk-stress GPU parallel loops.

[coderabbitai]

🧹 Nitpick comments (1)

src/simulation/m_viscous.fpp (1)

321-321: LGTM — consider adding q to the private clause to be fully explicit

The added variables (i, j, k, l, rho_visc, gamma_visc, pi_inf_visc, alpha_visc_sum) correctly mirror the 2D shear-stress loop at line 104 and fix the GPU race condition.

The integer q (declared at line 83) is used as the induction variable of the inner do q = 1, Re_size(i) seq sub-loop (lines 392–396). While OpenACC treats subroutine-local scalars as implicitly private in a parallel region, the project guideline calls for all loop-local variables to be listed explicitly. The same gap exists in the 2D counterpart at line 104.

💡 Suggested addition

-$:GPU_PARALLEL_LOOP(collapse=3, private='[i,j,k,l,rho_visc, gamma_visc, pi_inf_visc, alpha_visc_sum, alpha_visc, alpha_rho_visc, Re_visc, tau_Re]')
+$:GPU_PARALLEL_LOOP(collapse=3, private='[i,j,k,l,q,rho_visc, gamma_visc, pi_inf_visc, alpha_visc_sum, alpha_visc, alpha_rho_visc, Re_visc, tau_Re]')

(The same addition should be applied to the 2D shear/bulk loops at lines 104 and 214, and to the 3D bulk loop at line 430.)

Based on learnings: "Ensure private(...) declarations on all loop-local variables in GPU-accelerated code to prevent unintended data sharing."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/simulation/m_viscous.fpp` at line 321, Add the induction variable q to
the private clause of the GPU_PARALLEL_LOOP directives to explicitly mark it as
loop-local: update the existing GPU_PARALLEL_LOOP(collapse=3, private='[...]')
that governs the 3D shear-stress loop (and the analogous GPU_PARALLEL_LOOP
directives used for the 2D shear and bulk loops and the 3D bulk loop) so that
'q' is included among the private variables alongside
i,j,k,l,rho_visc,gamma_visc,pi_inf_visc,alpha_visc_sum,alpha_visc,alpha_rho_visc,Re_visc,tau_Re;
this ensures the inner "do q = 1, Re_size(i)" seq sub-loop uses a private
induction variable and satisfies the project guideline for explicit loop-local
private declarations.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/simulation/m_viscous.fpp`:
- Line 430: The OpenACC/OpenMP GPU_PARALLEL_LOOP directive missing the
inner-loop induction variable causes incorrect privatization; update the
directive that currently lists private='[i,j,k,l,rho_visc, gamma_visc,
pi_inf_visc, alpha_visc_sum, alpha_visc, alpha_rho_visc, Re_visc, tau_Re]' to
also include the inner seq-loop induction variable q so it matches the 2D
bulk-stress loop’s private clause and the inner seq loop that uses q (the inner
loop around lines 501–505 referenced in the review).

---

Nitpick comments:
In `@src/simulation/m_viscous.fpp`:
- Line 321: Add the induction variable q to the private clause of the
GPU_PARALLEL_LOOP directives to explicitly mark it as loop-local: update the
existing GPU_PARALLEL_LOOP(collapse=3, private='[...]') that governs the 3D
shear-stress loop (and the analogous GPU_PARALLEL_LOOP directives used for the
2D shear and bulk loops and the 3D bulk loop) so that 'q' is included among the
private variables alongside
i,j,k,l,rho_visc,gamma_visc,pi_inf_visc,alpha_visc_sum,alpha_visc,alpha_rho_visc,Re_visc,tau_Re;
this ensures the inner "do q = 1, Re_size(i)" seq sub-loop uses a private
induction variable and satisfies the project guideline for explicit loop-local
private declarations.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 44.03%. Comparing base (ab5082e) to head (1daf95b).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/common/m_phase_change.fpp	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1225      +/-   ##
==========================================
- Coverage   44.04%   44.03%   -0.01%     
==========================================
  Files          70       70              
  Lines       20499    20498       -1     
  Branches     1993     1994       +1     
==========================================
- Hits         9028     9026       -2     
  Misses      10330    10330              
- Partials     1141     1142       +1     

☔ 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.

[coderabbitai]

🧹 Nitpick comments (1)

src/simulation/m_viscous.fpp (1)

105-105: Minor: trailing space before comma in alpha_visc_sum ,alpha_visc.

Lines 105 and 215 have alpha_visc_sum ,alpha_visc (space before the comma), which is inconsistent with the standard alpha_visc_sum, alpha_visc formatting used in the corresponding 3D blocks at lines 322 and 431.

✏️ Proposed fix (both occurrences)

-$:GPU_PARALLEL_LOOP(collapse=3, private='[i,j,k,l,q,rho_visc, gamma_visc, pi_inf_visc, alpha_visc_sum ,alpha_visc, alpha_rho_visc, Re_visc, tau_Re]')
+$:GPU_PARALLEL_LOOP(collapse=3, private='[i,j,k,l,q,rho_visc, gamma_visc, pi_inf_visc, alpha_visc_sum, alpha_visc, alpha_rho_visc, Re_visc, tau_Re]')

Also applies to: 215-215

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/simulation/m_viscous.fpp` at line 105, Fix the minor formatting typo in
the GPU_PARALLEL_LOOP private list: remove the stray space before the comma so
the variables read "alpha_visc_sum, alpha_visc" (consistent with the 3D blocks);
update both occurrences where "alpha_visc_sum ,alpha_visc" appears (lines around
the GPU_PARALLEL_LOOP uses) so the private clause lists "alpha_visc_sum,
alpha_visc" exactly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/simulation/m_viscous.fpp`:
- Line 105: Fix the minor formatting typo in the GPU_PARALLEL_LOOP private list:
remove the stray space before the comma so the variables read "alpha_visc_sum,
alpha_visc" (consistent with the 3D blocks); update both occurrences where
"alpha_visc_sum ,alpha_visc" appears (lines around the GPU_PARALLEL_LOOP uses)
so the private clause lists "alpha_visc_sum, alpha_visc" exactly.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0e892dc and 164cda3.

📒 Files selected for processing (1)

• src/simulation/m_viscous.fpp

[wilfonba]

Looks good to me

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7658b43 and dca6c2f.

📒 Files selected for processing (1)

• src/simulation/m_viscous.fpp

---

📝 Walkthrough

Walkthrough

The changes modify src/simulation/m_viscous.fpp to expand private variable lists within GPU_PARALLEL_LOOP blocks, adding loop variables such as q and i/j/k/l to improve variable scoping in parallel regions. Additionally, several inline comments are corrected for spelling and grammar, including fixes like "diriviatives" to "derivatives," "spacial" to "spatial," and "equaiont" to "equation." The total modifications amount to six lines added and six removed, with no alterations to exported or public entity declarations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Whiskers twitch with giddy delight!
Variables scoped, GPU loops burning bright,
"Derivatives" spelled right, no more "spacial" mix,
Comments all tidied—a grammatical fix! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The description includes detailed explanation of the issue and fix, but lacks explicit completion of required template sections like type of change checkbox, testing confirmation, and GPU checklist items.	Complete the required template sections: mark the type of change (Bug fix), confirm testing method for GPU results, and check the GPU-specific testing checklist items.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: adding missing private clauses to 3D viscous GPU loops to fix data race conditions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.}

[github-actions]

Claude Code Review

Head SHA: a2c4d1d

Files changed: 2

• src/simulation/m_viscous.fpp
• src/common/m_phase_change.fpp

---

Summary

• Adds missing rho_visc, gamma_visc, pi_inf_visc, alpha_visc_sum, and q to the private clauses of the 3D shear/bulk stress GPU_PARALLEL_LOOP directives in m_viscous.fpp, fixing genuine GPU race conditions in 3D viscous simulations.
• Adds q to the private clauses of the corresponding 2D loops (lines 102, 212) for consistency and correctness.
• Replaces a matmul intrinsic call with explicit 2x2 element-wise computation in m_phase_change.fpp.
• Fixes three comment typos: diriviatives → derivatives, equaiont → equation, spacial → spatial.

---

Findings

[MINOR] m_phase_change.fpp:580-581 — Undocumented change, needs explanation

The replacement of:

DeltamP = -1.0_wp*(matmul(InvJac, R2D))

with explicit element-wise 2x2 matrix-vector multiplication is mathematically correct but is not mentioned in the PR description. This change likely fixes a GPU-device-code incompatibility (some compilers/backends do not support matmul inside GPU_ROUTINE or GPU kernel contexts), but the motivation should be documented. Since m_phase_change.fpp is in src/common/, this change affects all three executables. Please confirm this is intentional and add a note to the PR description.

[CONFIRM] 3D loop fix is correct and matches 2D

The fix at lines 319 and 428 of m_viscous.fpp aligns the 3D private clause with what the 2D loops already had. The collapse=3 over (l, k, j) is correct for the loop structure. The added variables are genuine per-thread temporaries (computed from per-cell alpha arrays) that must be private to avoid data races on GPU.

[TRIVIAL] Trailing space removed from private clause

At lines 102 and 212, the old clause had alpha_visc_sum ,alpha_visc (space before comma). The fix also cleans this up — no correctness issue.

---

Overall

The core race-condition fix is correct and well-motivated. The only item worth addressing before merge is documenting the matmul → explicit replacement in m_phase_change.fpp.

[github-actions]

Claude Code Review

Head SHA: 1daf95b8873ce2ac1652a7ef8ecc857c75508ba5

Files changed: 2

• src/simulation/m_viscous.fpp (+6/-6)
• src/common/m_phase_change.fpp (+4/-2)

---

Summary

• Adds missing loop indices (i,j,k,l,q) and viscous temporaries (rho_visc, gamma_visc, pi_inf_visc, alpha_visc_sum) to the private clause of 3D GPU parallel loops in m_viscous.fpp — a real GPU race-condition fix.
• Adds q to the existing private clause of the 2D loops and removes a stray space (cosmetic).
• Replaces matmul with explicit element-wise 2×2 arithmetic in m_phase_change.fpp.
• Restructures a do while loop to do … exit in m_phase_change.fpp.
• Two comment typo fixes (diriviatives→derivatives, spacial→spatial, equaiont→equation).

---

Findings

[Medium] m_phase_change.fpp changes are undescribed and in src/common/

src/common/m_phase_change.fpp is shared by all three executables (pre_process, simulation, post_process). The PR description covers only m_viscous.fpp. The two m_phase_change changes need rationale and require testing all three targets per project rules.

[Low] matmul → explicit arithmetic needs rationale (src/common/m_phase_change.fpp ~L582)

The arithmetic is correct for a 2×2 system. However, the motivation is unexplained. If this is because matmul is not supported in GPU device code (e.g., inside an OpenACC/OpenMP routine), that should be documented with a comment. If it is a standalone correctness fix, the PR description should say so.

[Low] do while → do … exit restructuring (src/common/m_phase_change.fpp ~L717)

The semantics are equivalent (the ns == 0 guard in the original only ensured one mandatory iteration, which the new do … exit also guarantees). The change is acceptable but unexplained in the PR body and should be noted.

---

Improvement Opportunities

• The 3D private-clause fix is the right approach, but it would be worth a follow-up audit of all other GPU_PARALLEL_LOOP sites in m_viscous.fpp to confirm no other loops are missing loop-index or scalar-temporary privatization.
• A 3D GPU viscous regression test (e.g., 3D channel flow or Couette flow with Re > 0) should be explicitly called out in the test plan to prevent future regressions of this class of bug.

[github-actions]

Claude Code Review

Head SHA: 1daf95b8873ce2ac1652a7ef8ecc857c75508ba5

Files changed: 2

• src/simulation/m_viscous.fpp (+6/-6)
• src/common/m_phase_change.fpp (+4/-2)

---

Summary

• Adds missing loop indices (i,j,k,l,q) and viscous temporaries (rho_visc, gamma_visc, pi_inf_visc, alpha_visc_sum) to the private clause of 3D GPU parallel loops in m_viscous.fpp — a real GPU race-condition fix.
• Adds q to the existing private clause of the 2D loops and removes a stray space (cosmetic).
• Replaces matmul with explicit element-wise 2×2 arithmetic in m_phase_change.fpp.
• Restructures a do while loop to do … exit in m_phase_change.fpp.
• Two comment typo fixes (diriviatives→derivatives, spacial→spatial, equaiont→equation).

---

Findings

[Medium] m_phase_change.fpp changes are undescribed and in src/common/

src/common/m_phase_change.fpp is shared by all three executables (pre_process, simulation, post_process). The PR description covers only m_viscous.fpp. The two m_phase_change changes need rationale and require testing all three targets per project rules.

[Low] matmul → explicit arithmetic needs rationale (src/common/m_phase_change.fpp ~L582)

The arithmetic is correct for a 2×2 system. However, the motivation is unexplained. If this is because matmul is not supported in GPU device code (e.g., inside an OpenACC/OpenMP routine), that should be documented with a comment. If it is a standalone correctness fix, the PR description should say so.

[Low] do while → do … exit restructuring (src/common/m_phase_change.fpp ~L717)

The semantics are equivalent (the ns == 0 guard in the original only ensured one mandatory iteration, which the new do … exit also guarantees since the exit is at the end of the loop body). The change is acceptable but unexplained in the PR body and should be noted.

---

Improvement Opportunities

• The 3D private-clause fix is correct, but a follow-up audit of all other GPU_PARALLEL_LOOP sites in m_viscous.fpp would confirm no other loops have missing index or scalar-temporary privatization.
• A 3D GPU viscous regression test (e.g., 3D channel flow or Couette flow with Re > 0) should be explicitly called out in the test plan to prevent future regressions of this class of bug.