Retire the WENO7 amdflang workaround; record which ones AFAR 24.3 still needs - #1939
Merged
sbryngelson merged 1 commit intoOct 5, 2026
Merged
Conversation
…ll needs - Restore the reversed-stride sections in the WENO7 coefficients (MFlowCode#1660). The false-nuw fix (llvm/llvm-project#198014) is in AFAR 24.3. - Note that the attributor cap flag (MFlowCode#1759) and HLLC's split call site are still needed on 24.3 (cap unchanged, ROCm/llvm-project#4070; merging the call sites faults every HLLC case). - Correct the docs: on 24.3 the USING_AMD guards are needed for performance (4-5x), not correctness; note the BLOCK reproducer.
Lines of Code
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The restored expressions are equivalent to the element-wise assignments, and the documentation accurately reflects the retained workarounds.
Review effort: Balanced
Findings: None
What changed in this PR
Retires the obsolete WENO7 amdflang workaround and documents the remaining AFAR 24.3 constraints.
Changes:
- Restores concise reversed-stride WENO7 coefficient assignments.
- Records retained HLLC and linker workarounds.
- Updates AMD performance and
blockguidance.
| File | Description |
|---|---|
src/simulation/m_weno.fpp |
Restores reversed-stride WENO7 sections. |
src/simulation/m_riemann_solver_hllc.fpp |
Notes the HLLC workaround remains necessary. |
docs/documentation/gpuParallelization.md |
Documents current AFAR 24.3 findings. |
cmake/MFCTargets.cmake |
Records why the Attributor override remains. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1939 +/- ##
==========================================
- Coverage 62.80% 62.77% -0.04%
==========================================
Files 86 86
Lines 22385 22366 -19
Branches 3304 3304
==========================================
- Hits 14060 14041 -19
Misses 6073 6073
Partials 2252 2252 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Summary
After the move to AFAR 24.3.0 (#1920), this checks which amdflang workarounds are still needed, removes the one that isn't, and records the results.
nuwfix, llvm/llvm-project#198014, is in the drop's source; the WENO7 tests pass at-O3-attributor-max-pi-accesses=16384(#1759)cl::init(512)), and the GPU image has 486 kernels against a threshold of about 512 (ROCm/llvm-project#4070, fix proposed in #4094)GPU_PARALLEL_LOOPcall siteUSING_AMDfixed-bound guardsrocprofv3)BLOCKRe_sizehost copies (#1588)Testing
On HPCFund MI210 (gfx90a), AFAR 24.3.0, amdflang
--gpu mprelease build (-O3), no case optimization:./mfc.sh benchagainst master on the same node: geometric-mean speed ratio 1.006, every case within ±2%.These runs used a build that also read
Re_sizedirectly. That change was dropped afterwards for the Cray reason above, so the Riemann solvers are back to master's code and this PR's only code change is the WENO7 revert. The WENO7 revert affects every compiler, so CI covers it on each one.Results are unchanged: every test matches its golden file.
This PR was prepared with Claude Code (AI-assisted).
Acknowledgement