[Experimental] Fixed device-array bounds for all compilers (USING_AMD = True) - #1938
Open
sbryngelson wants to merge 1 commit into
Open
sbryngelson wants to merge 1 commit into
sbryngelson wants to merge 1 commit into
Conversation
… True) Measures in CI whether the compile-time bounds behind the USING_AMD guards speed up non-case-optimized NVHPC, CCE, and GNU builds as they do amdflang (3.8-5.3x on AFAR 24.3). Not for merge.
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It is explicitly a do-not-merge experiment pending cross-compiler benchmark results.
Review effort: Balanced
Findings: None
What changed in this PR
This experimental PR enables AMD-style fixed array bounds across all compilers to collect comparative CI benchmarks.
Changes:
- Sets
USING_AMDunconditionally for fixed bounds and associated runtime limits. - Leaves compiler-specific OpenMP directive selection unchanged.
| File | Description |
|---|---|
src/common/include/shared_parallel_macros.fpp |
Enables fixed device-array bounds for every compiler. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Claude Code Review Head SHA: 55c5708 Files changed:
Findings:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1938 +/- ##
==========================================
- Coverage 62.82% 62.70% -0.13%
==========================================
Files 86 86
Lines 22394 22311 -83
Branches 3305 3305
==========================================
- Hits 14070 13989 -81
+ Misses 6071 6069 -2
Partials 2253 2253 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 task
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Experiment — do not merge. Opened to get CI benchmark numbers on NVHPC, CCE, and GNU.
Question
Do the fixed device-array bounds behind the
USING_AMDguards speed up the other compilers too, without case optimization?Why
On AFAR 24.3 (amdflang, MI210), turning the guards off gives correct results but makes AMD 3.8–5.3x slower on
./mfc.sh bench(master vs guards-off, same node):rocprofv3on5eq_rk3_weno3_hllcputs 98% of the added kernel time in two places. In the WENO kernel (m_weno.fpp:1005, guard at:921), the privatepoly/alpha/omega/beta/dvdarrays become runtime-sized: scratch goes 0 → 400 B per thread and the kernel is 11x slower. In HLLC (l899), thenum_fluids/sys_sizearrays make it 4.9x slower. With compile-time bounds the arrays stay in registers.Non-case-optimized NVHPC and CCE builds have always used the runtime-sized arrays. This PR checks whether they pay a similar cost.
Change
One line:
USING_AMD = Truefor every compiler. This turns on the guarded fixed bounds (3 fluids,nb≤ 3, 60 species,sys_size≤ 70, WENO0:4, QBMM 32) and the matchings_check_amdlimits for all non-case-optimized builds. OpenMP directive selection (omp_macros.fpp) checks the compiler ID directly, so it is unchanged.If the numbers favor it, the real PR will rename the guards so they no longer say AMD, keep the limits as named maxima, and leave case optimization as the way past them.
Verification
This PR was prepared with Claude Code (AI-assisted). The measurements were run on HPCFund MI210 nodes with AFAR 24.3.0.
Acknowledgement