WENO/MUSCL case guard w/ recon_type number + Inlining typo causing segmentation faults on compilation#1379
WENO/MUSCL case guard w/ recon_type number + Inlining typo causing segmentation faults on compilation#1379Cowsreal wants to merge 8 commits intoMFlowCode:masterfrom
Conversation
|
@claude full review |
📝 WalkthroughWalkthroughThe pull request restricts emission of Cray-specific directives in 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
toolchain/mfc/case_validator.py (1)
128-137: Optionally updatePHYSICS_DOCSentries to document the new cross-compatibility rules.The contributing guide states that
PHYSICS_DOCSfeeds auto-generated physics-constraints docs. The new rules ("recon_type = 1 rejects MUSCL params" / "recon_type = 2 rejects WENO params and requires weno_order unset or 0") aren't reflected in the explanations forcheck_weno/check_muscl. Consider appending a sentence to each so the auto-generated docs stay in sync.📝 Suggested doc additions
"check_weno": { "title": "WENO Reconstruction", "category": "Numerical Schemes", - "explanation": ("weno_order must be 1, 3, 5, or 7. Grid must have enough cells. Only one of mapped_weno, wenoz, teno can be active."), + "explanation": ("weno_order must be 1, 3, 5, or 7. Grid must have enough cells. Only one of mapped_weno, wenoz, teno can be active. When recon_type = 1 (WENO), MUSCL parameters (muscl_order, muscl_lim) must not be set."), }, "check_muscl": { "title": "MUSCL Reconstruction", "category": "Numerical Schemes", - "explanation": "muscl_order must be 1 or 2. Second order requires muscl_lim in {1,2,3,4,5}.", + "explanation": "muscl_order must be 1 or 2. Second order requires muscl_lim in {1,2,3,4,5}. When recon_type = 2 (MUSCL), WENO-specific parameters must not be enabled and weno_order must be unset or 0.", },As per coding guidelines (contributing.md): cross-parameter validation rules should be reflected in
PHYSICS_DOCSso auto-generated physics-constraints docs stay accurate.src/common/include/parallel_macros.fpp (1)
66-103: Cray directive gating looks correct.Both branches now emit
!DIR$ NOINLINE/!DIR$ INLINEALWAYSonly inside#ifdef _CRAYFTN, while non-Cray OpenACC/OpenMP paths fall through to$:acc_directive/$:omp_directivevia the outer#elifarms. This should resolve the Cray+OpenMP Frontier compile failure without affecting NVHPC/GNU/Intel code paths.Two small readability nits (optional):
- Lines 77–78 and 97–98: the empty
#else/#endifinside the inner#if MFC_OpenACC …#elifMFC_OpenMPis a no-op and can be dropped for clarity.- Lines 79–80: the Fypp comment describing "non-Cray CPU builds … nothing is emitted" is placed inside the
#ifdef _CRAYFTNbranch, which is where non-Cray builds are by definition not taken. Consider relocating it above the#ifdef _CRAYFTNor next to the outer#endifso the intent reads correctly.🧹 Proposed minor cleanup for cray_inline branch (mirror for cray_noinline)
`#ifdef` _CRAYFTN $:cray_directive `#if` MFC_OpenACC $:acc_directive `#elif` MFC_OpenMP $:omp_directive -#else `#endif` `#elif` MFC_OpenACC $:acc_directive `#elif` MFC_OpenMP $:omp_directive `#endif`Please confirm with a Cray+OpenMP build on Frontier that
phase_change.fppnow compiles cleanly and that NVHPC+OpenACC and GNU CPU-only builds are unaffected (the change touches the sharedGPU_ROUTINEpath used across compilers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3500d2bd-6664-4d2a-ae6f-1de6eff829c2
📒 Files selected for processing (2)
src/common/include/parallel_macros.fpptoolchain/mfc/case_validator.py
|
Issue: MUSCL guard add certain real valued WENO parameters, that have default values set in BASE_CFG which don't fail the test cases. Solution: In the corresponding test cases, push None for those parameters, then pop them out inside the TestCase constructor before they trip schema. This does not interfere wiht any already existing test cases since no one else uses None. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
/review |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/common/include/parallel_macros.fpp (1)
95-107: Optional: mirror the cray_noinline explanatory comment and drop the empty#else/#endif.The
cray_inlinebranch is structurally symmetric tocray_noinline, but:
- Lines 101-102 contain an empty
#else/#endifpair that doesn't emit anything. It's dead code (the#if MFC_OpenACC/#elif MFC_OpenMPabove it doesn't need an#else).- The equivalent slot in the
cray_noinlinebranch (lines 81-82) carries a comment explaining why nothing is emitted on non-Cray CPU builds — worth mirroring here for future readers of the macro.♻️ Suggested tidy-up
`#ifdef` _CRAYFTN $:cray_directive `#if` MFC_OpenACC $:acc_directive `#elif` MFC_OpenMP $:omp_directive `#else` + #! On non-Cray CPU builds (no _CRAYFTN, no MFC_OpenACC, no MFC_OpenMP), nothing is + #! emitted — intentional, since !DIR$ INLINEALWAYS is a Cray-specific directive. `#endif` `#elif` MFC_OpenACC $:acc_directive `#elif` MFC_OpenMP $:omp_directive `#endif`(Alternatively drop the empty
#else/#endifentirely since the inner#if/#elifwithout an#elseis valid.)toolchain/mfc/test/cases.py (1)
303-303: LGTM on the sentinel-Noneapproach; minor readability suggestion.Pushing
weno_eps/wenoz_q/teno_CTasNonecorrectly pairs with the new filter inTestCase.__init__to stripBASE_CFGdefaults before validation — matches the approach described in the PR. Optional: consider wrapping this multi-key dict onto multiple lines (as done elsewhere in this file) to make the WENO-suppression intent more obvious to future readers.✏️ Optional readability tweak
- for muscl_order in [1, 2]: - stack.push(f"muscl_order={muscl_order}", {"muscl_order": muscl_order, "recon_type": 2, "weno_order": 0, "weno_eps": None, "wenoz_q": None, "teno_CT": None}) + for muscl_order in [1, 2]: + stack.push( + f"muscl_order={muscl_order}", + { + "muscl_order": muscl_order, + "recon_type": 2, + "weno_order": 0, + # Suppress BASE_CFG WENO defaults so check_muscl doesn't + # reject MUSCL cases; filtered out in TestCase.__init__. + "weno_eps": None, + "wenoz_q": None, + "teno_CT": None, + }, + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57b4b57a-10e2-4289-b7c2-a00af61c582f
📒 Files selected for processing (4)
src/common/include/parallel_macros.fpptoolchain/mfc/case_validator.pytoolchain/mfc/test/case.pytoolchain/mfc/test/cases.py
1 participant
Description
I ran a whole batch of cases with WENO parameters set while recon_type = 2 was also set. There is no case guard, so this resulted in a MUSCL order 0 simulation being ran, where the toolchain should have warned the user.
I kept running into segmentation faults with phase_change.fpp while trying to compile using OpenMP + CCE 19 on Frontier. The phase change module extensively uses the $:GPU_ROUTINE macro, specifically, the cray_noinline option. It's currently doing nothing when it evaluates the _CRAYFTN branch. I was able to compile after fixing this typo inside the parallel_macros.fpp.
Type of change
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)claude-full-review— Claude full review via label