Skip to content

Remove Non-parameter workaround for Chemistry cases with AMD#1446

Merged
sbryngelson merged 17 commits into
MFlowCode:masterfrom
anandrdbz:master
May 19, 2026
Merged

Remove Non-parameter workaround for Chemistry cases with AMD#1446
sbryngelson merged 17 commits into
MFlowCode:masterfrom
anandrdbz:master

Conversation

@anandrdbz
Copy link
Copy Markdown
Contributor

@anandrdbz anandrdbz commented May 18, 2026

Description

Removes the non-parameter workaround for chemistry with AMD compilers, allowing to run chemistry cases with variable num_species. No fixes are needed on Pyrometheus. @DimAdam-01 @sbryngelson

In addition, the PR removes run_time_info from the igr benchmark case as this should've never been set to true

Verified all chemistry test cases pass on frontier with AMD compilers

Fixes #(issue)

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

Anand and others added 17 commits May 12, 2026 15:42
…elocation error

flang-23/LLD defaults to building PIE executables. SILO and LAPACK static
libraries on Frontier are compiled without -fPIC, so their 32-bit absolute
relocations (R_X86_64_32) are rejected by LLD when linking a PIE binary.
Add -no-pie to post_process link options for LLVMFlang to allow non-PIC
system libraries. simulation is unaffected (no SILO/LAPACK dependency).
… limit

AMD flang case-opt compilation takes close to the 2h hackathon wall limit,
leaving no time for the run step. Split into two sequential hackathon GPU jobs:
1. Pre-Build: compiles all benchmarks via --dry-run (build only, no execution)
2. Run: skips build (binaries cached), runs and validates benchmarks

Also preserve dependency dirs in prebuild for non-Phoenix clusters (deps are
already built by the Fetch Dependencies step, so only clean staging dirs).
@anandrdbz anandrdbz requested a review from sbryngelson as a code owner May 18, 2026 20:20
@qodo-code-review
Copy link
Copy Markdown
Contributor

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@github-actions
Copy link
Copy Markdown

Claude Code Review

Head SHA: 904b5a7

Files changed:

  • 7
  • benchmarks/igr/case.py
  • src/common/m_checker_common.fpp
  • src/common/m_chemistry.fpp
  • src/simulation/include/inline_riemann.fpp
  • src/simulation/m_cbc.fpp
  • src/simulation/m_riemann_solvers.fpp
  • src/simulation/m_start_up.fpp

Findings:

[GPU Correctness – AMD] molecular_weights device initialization no longer guaranteed after removing the AMD initialization block

src/simulation/m_start_up.fpp removes the block that was the sole mechanism transferring molecular_weights data to the AMD GPU device:

! Removed:
#:if USING_AMD
    block
        use m_thermochem, only: molecular_weights
        use m_chemistry, only: molecular_weights_nonparameter
        molecular_weights_nonparameter(:) = molecular_weights(:)
        $:GPU_UPDATE(device='[molecular_weights_nonparameter]')
    end block
#:endif

And src/common/m_chemistry.fpp removes the module-level GPU_DECLARE that backed it:

! Removed:
#:if USING_AMD
    real(wp) :: molecular_weights_nonparameter(10) = ...
    $:GPU_DECLARE(create='[molecular_weights_nonparameter]')
#:endif

After this PR, every AMD GPU kernel in m_chemistry.fpp, m_cbc.fpp, m_riemann_solvers.fpp, and inline_riemann.fpp uses molecular_weights from m_thermochem directly inside GPU_PARALLEL_LOOP blocks. This is only correct if m_thermochem has its own GPU_DECLARE for molecular_weights and a corresponding GPU_UPDATE(device=...) to populate it before the first kernel launch. Neither is visible in the changed files. If molecular_weights is not device-resident on AMD, the GPU kernels will silently read uninitialised or stale device memory, producing wrong species reaction rates, enthalpies, and mole fractions without any runtime error.

Please confirm that m_thermochem (not changed in this PR) has $:GPU_DECLARE(create='[molecular_weights]') and a matching $:GPU_UPDATE(device='[molecular_weights]') call that executes before s_initialize_gpu_vars returns.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2026

Codecov Report

❌ Patch coverage is 60.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.31%. Comparing base (dd231ae) to head (904b5a7).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/simulation/m_cbc.fpp 0.00% 2 Missing ⚠️
src/simulation/m_riemann_solvers.fpp 50.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1446   +/-   ##
=======================================
  Coverage   61.31%   61.31%           
=======================================
  Files          72       72           
  Lines       19771    19771           
  Branches     2852     2852           
=======================================
  Hits        12123    12123           
  Misses       5699     5699           
  Partials     1949     1949           

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

@sbryngelson sbryngelson merged commit 45cd6bb into MFlowCode:master May 19, 2026
91 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants