Code Clean Up and Post-Processing Compilation Fix #1083
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR removes deprecated parallel loop macros ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
codeant-ai
bot
added
the
size:L
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| q_comm(i)%sf(j + unpack_offset, k, l) = real(buff_recv(r), kind=stp) | ||
| #if defined(__INTEL_COMPILER) | ||
| if (ieee_is_nan(q_comm(i)%sf(j, k, l))) then | ||
| print *, "Error", j, k, l, i | ||
| error stop "NaN(s) in recv" | ||
| end if | ||
| if (ieee_is_nan(q_comm(i)%sf(j, k, l))) then | ||
| print *, "Error", j, k, l, i | ||
| error stop "NaN(s) in recv" | ||
| end if | ||
| #endif |
There was a problem hiding this comment.
Suggestion: Fix the NaN check to use the correct array index with unpack_offset to validate the newly assigned value. [possible issue, importance: 8]
| q_comm(i)%sf(j + unpack_offset, k, l) = real(buff_recv(r), kind=stp) | |
| #if defined(__INTEL_COMPILER) | |
| if (ieee_is_nan(q_comm(i)%sf(j, k, l))) then | |
| print *, "Error", j, k, l, i | |
| error stop "NaN(s) in recv" | |
| end if | |
| if (ieee_is_nan(q_comm(i)%sf(j, k, l))) then | |
| print *, "Error", j, k, l, i | |
| error stop "NaN(s) in recv" | |
| end if | |
| #endif | |
| q_comm(i)%sf(j + unpack_offset, k, l) = real(buff_recv(r), kind=stp) | |
| #if defined(__INTEL_COMPILER) | |
| if (ieee_is_nan(q_comm(i)%sf(j + unpack_offset, k, l))) then | |
| print *, "Error", j, k, l, i | |
| error stop "NaN(s) in recv" | |
| end if | |
| #endif |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| print *, "Error", j, k, l, i | ||
| error stop "NaN(s) in recv" | ||
| end if | ||
| if (ieee_is_nan(q_comm(i)%sf(j, k, l))) then |
There was a problem hiding this comment.
Suggestion: Logic error: the NaN check tests a different array element than the one assigned (it omits unpack_offset), so a NaN written into the target element can go undetected and the check may read an unrelated location (possibly out of bounds); make the check use the same index j + unpack_offset. [logic error]
Severity Level: Minor
| if (ieee_is_nan(q_comm(i)%sf(j, k, l))) then | |
| if (ieee_is_nan(q_comm(i)%sf(j + unpack_offset, k, l))) then |
Why it matters? ⭐
The check currently reads a different location (j,k,l) than the element just written (j+unpack_offset,k,l), so it can miss NaNs or read unrelated memory. Changing the nan-test to inspect q_comm(i)%sf(j+unpack_offset,k,l) fixes a real logic bug and validates the value just written.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/common/m_mpi_common.fpp
**Line:** 970:970
**Comment:**
*Logic Error: Logic error: the NaN check tests a different array element than the one assigned (it omits `unpack_offset`), so a NaN written into the target element can go undetected and the check may read an unrelated location (possibly out of bounds); make the check use the same index `j + unpack_offset`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| print *, "Error", j, k, l, i | ||
| error stop "NaN(s) in recv" | ||
| end if | ||
| if (ieee_is_nan(q_comm(i)%sf(j, k, l))) then |
There was a problem hiding this comment.
Suggestion: Logic error: after unpacking the buffer in the mpi_dir==2 branch the code assigns into q_comm(i)%sf(j, k + unpack_offset, l) but the NaN check inspects q_comm(i)%sf(j, k, l), so it validates the wrong element; update the check to include k + unpack_offset. [logic error]
Severity Level: Minor
| if (ieee_is_nan(q_comm(i)%sf(j, k, l))) then | |
| if (ieee_is_nan(q_comm(i)%sf(j, k + unpack_offset, l))) then |
Why it matters? ⭐
Same class of bug as above: assignment writes into k+unpack_offset but the NaN check inspects k. Updating the checked index to k+unpack_offset ensures the freshly assigned element is validated and prevents false negatives.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/common/m_mpi_common.fpp
**Line:** 1025:1025
**Comment:**
*Logic Error: Logic error: after unpacking the buffer in the mpi_dir==2 branch the code assigns into `q_comm(i)%sf(j, k + unpack_offset, l)` but the NaN check inspects `q_comm(i)%sf(j, k, l)`, so it validates the wrong element; update the check to include `k + unpack_offset`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| print *, "Error", j, k, l, i | ||
| error stop "NaN(s) in recv" | ||
| end if | ||
| if (ieee_is_nan(q_comm(i)%sf(j, k, l))) then |
There was a problem hiding this comment.
Suggestion: Logic error: in the z-direction unpacking the code writes into q_comm(i)%sf(j, k, l + unpack_offset) but the NaN test reads q_comm(i)%sf(j, k, l), so the validation misses the element that was just written; change the check to use l + unpack_offset. [logic error]
Severity Level: Minor
| if (ieee_is_nan(q_comm(i)%sf(j, k, l))) then | |
| if (ieee_is_nan(q_comm(i)%sf(j, k, l + unpack_offset))) then |
Why it matters? ⭐
The code writes into l+unpack_offset but tests l, so it doesn't validate the written element. Adjusting the NaN check to l+unpack_offset fixes a real correctness issue.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/common/m_mpi_common.fpp
**Line:** 1084:1084
**Comment:**
*Logic Error: Logic error: in the z-direction unpacking the code writes into `q_comm(i)%sf(j, k, l + unpack_offset)` but the NaN test reads `q_comm(i)%sf(j, k, l)`, so the validation misses the element that was just written; change the check to use `l + unpack_offset`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
💡 Enhance Your PR ReviewsWe noticed that 3 feature(s) are not configured for this repository. Enabling these features can help improve your code quality and workflow: 🚦 Quality GatesStatus: Quality Gates are not enabled at the organization level 🎫 Jira Ticket ComplianceStatus: Jira credentials file not found. Please configure Jira integration in your settings ⚙️ Custom RulesStatus: No custom rules configured. Add rules via organization settings or .codeant/review.json in your repository Want to enable these features? Contact your organization admin or check our documentation for setup instructions. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1083 +/- ##
==========================================
- Coverage 44.16% 44.11% -0.06%
==========================================
Files 71 71
Lines 20197 20250 +53
Branches 1970 1970
==========================================
+ Hits 8921 8933 +12
- Misses 10148 10189 +41
Partials 1128 1128 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sbryngelson I think that this is ready for merge |
2 participants
User description
User description
Description
I became aware that post_process does not compile with the
--no-mpiflag right now. Even through we don't use post process without parallel IO, I still wanted to resolve the compilation so that it doesn't throw bugs or confuse anyone.I also noticed that I missed about 6 macro calls from the old macros, so I removed them and cleaned up the code a little.
Type of change
Please delete options that are not relevant.
Scope
If you cannot check the above box, please split your PR into multiple PRs that each have a common goal.
How Has This Been Tested?
It runs the test suite locally. These changes should not be breaking.
Test Configuration:
Checklist
docs/)examples/that demonstrate my new feature performing as expected.They run to completion and demonstrate "interesting physics"
./mfc.sh formatbefore committing my codeIf your code changes any code source files (anything in
src/simulation)To make sure the code is performing as expected on GPU devices, I have:
PR Type
Bug fix, Enhancement
Description
Removed deprecated
_OLDmacro variants from GPU parallelization macrosFixed post-processing compilation with
--no-mpiflag by adding conditional MPI guardsReplaced all
GPU_PARALLEL_LOOP_OLDcalls withGPU_PARALLEL_LOOPin MPI communication codeImproved code consistency and removed redundant macro definitions
Diagram Walkthrough
File Walkthrough
acc_macros.fpp
Remove deprecated ACC_PARALLEL_LOOP_OLD macrosrc/common/include/acc_macros.fpp
ACC_PARALLEL_LOOP_OLDmacro definitionACC_PARALLEL_LOOPmacro intact for current usageomp_macros.fpp
Remove deprecated OMP_PARALLEL_LOOP_OLD macrosrc/common/include/omp_macros.fpp
OMP_PARALLEL_LOOP_OLDmacro definitionOMP_PARALLEL_LOOPmacro intact for current usagelogic
parallel_macros.fpp
Remove deprecated GPU_PARALLEL_LOOP_OLD macrosrc/common/include/parallel_macros.fpp
GPU_PARALLEL_LOOP_OLDmacro definitionGPU_PARALLEL_LOOPmacro intact for current usagem_mpi_common.fpp
Replace deprecated GPU_PARALLEL_LOOP_OLD callssrc/common/m_mpi_common.fpp
GPU_PARALLEL_LOOP_OLDcalls withGPU_PARALLEL_LOOP#:callto$:notation$:END_GPU_PARALLEL_LOOP()closing statementsm_start_up.fpp
Add MPI conditional compilation guardssrc/post_process/m_start_up.fpp
#ifdef MFC_MPIguards around MPI-dependent code sectionsMPI_ALLREDUCEcall in energy FFT computations_mpi_transpose_x2yands_mpi_transpose_y2zs_initialize_modulesands_initialize_mpi_domains_finalize_modules--no-mpiflagCodeAnt-AI Description
Allow post-processing to compile and run without MPI and remove legacy parallel macros
What Changed
Impact
✅ Builds post-processing in non-MPI (serial) configurations✅ Fewer compilation failures caused by leftover legacy macros✅ Consistent generated parallel regions during communication unpacking💡 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:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
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:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
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.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.