Conversation
…o levelset-refactor
…o levelset-refactor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/simulation/m_compute_levelset.fpp`:
- Around line 239-240: z_min/z_max are computed in global coordinates (using
center(3)) but later compared to the already-translated local coordinate
xyz_local(3), causing wrong distances and normals; change the code so z bounds
are expressed in the local frame (e.g. z_min = -lz/2 and z_max = +lz/2) or
compute z_min/z_max after subtracting center (so they are relative to the
translated origin) and then use those when computing dist_side and normals for
xyz_local(3). Update the analogous occurrence around line ~288 as well (keep
references: z_min, z_max, center, lz, xyz_local, dist_side).
- Around line 64-66: The 2D GPU_PARALLEL_LOOP uses copy='[gps]' and
patch_ib(1:num_ibs) which is inconsistent with the 3D path; change the 2D loop's
data clauses to use present='[gps]' (since gps is already on-device via
GPU_UPDATE) and make the patch_ib clause consistent with the 3D loop (use the
full patch_ib array form rather than the sliced (1:num_ibs)), leaving
private='[i,patch_id,patch_geometry]' and copyin='[Np,patch_ib]' (or the same
copyin form used in the 3D branch) so the kernel uses present gps and a
consistent patch_ib specifier.
- Around line 39-60: The GPU loops call s_model_levelset which dereferences the
host array-of-structs models (e.g. models(patch_id)%interpolate,
%boundary_edge_count, %total_vertices, %model, %interpolated_boundary_v,
%boundary_v) but models is not included in the GPU data clauses; add models to
device data so the kernel won't fault. Best fix: after
s_instantiate_STL_models() returns, create a persistent GPU data region (using
$:GPU_ENTER_DATA(present='[models]', copyin='[models]') / $:END_GPU_ENTER_DATA()
or the project macro equivalent) to keep models on-device for reuse;
alternatively, add models to the copyin list of the $:GPU_PARALLEL_LOOP(...)
that calls s_model_levelset (and the corresponding 2D loop) so models is present
during the loops. Ensure the same symbol name models is used in the data clause
and that its lifetime covers both loops.
In `@src/simulation/m_ibm.fpp`:
- Around line 72-73: s_finalize_ibm_module must free the allocatable array
models and each of its allocatable components to avoid host/device leaks: in
s_finalize_ibm_module iterate over models (if allocated(models)) and for each
element check and deallocate its allocatable fields model, boundary_v, and
interpolated_boundary_v (use ALLOCATED() guards), then deallocate the models
array itself (again guarding with ALLOCATED()) and handle any device-specific
deallocation if these components were moved to the device; update
s_finalize_ibm_module to perform these checks and DEALLOCATE calls for model,
boundary_v, interpolated_boundary_v, then DEALLOCATE(models).
🧹 Nitpick comments (3)
src/simulation/m_compute_levelset.fpp (2)
557-559: Usewpreal literals in array constructors for type consistency.
gp%levelset_norm = (/1, 0, 0/)creates an integer array that gets implicitly converted. While Fortran handles this correctly for these values, using explicit real literals is more consistent with the rest of the file (e.g., line 292 uses0._wp).The same pattern appears in
s_cylinder_levelset(lines 601–612) with(/1, 0, 0/),(/0, 1, 0/), etc.✏️ Example fix for sphere
- gp%levelset_norm = (/1, 0, 0/) + gp%levelset_norm = (/1._wp, 0._wp, 0._wp/)
310-311: Stale docstring:s_rectangle_levelsetis labeled "Initialize IBM module".The
!> Initialize IBM modulecomment on line 310 is copy-paste from a different subroutine and does not describe what this routine does.✏️ Fix docstring
- !> Initialize IBM module - subroutine s_rectangle_levelset(gp) + !> Computes the levelset distance and normal for a rectangle patch + subroutine s_rectangle_levelset(gp)src/simulation/m_ibm.fpp (1)
97-98: Remove the commented-outGPU_ENTER_DATAline.The commented-out directive on line 97 is dead code. The
GPU_UPDATEon line 98 is the intended replacement—clean up the leftover.🧹 Proposed fix
- ! $:GPU_ENTER_DATA(copyin='[patch_ib(1:num_ibs)]') $:GPU_UPDATE(device='[patch_ib(1:num_ibs)]')
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/common/m_model.fpp (3)
1125-1152:⚠️ Potential issue | 🟠 MajorVLA
dist_bufferinside aGPU_ROUTINE(seq)function may fail on GPU devices.Line 1136 declares
dist_buffer(1:boundary_edge_count)as an automatic (stack) array with a runtime size. GPU device stacks are typically very limited, and some compilers (nvfortran) may reject or silently miscompile VLAs in device code. This can be eliminated by tracking the running minimum directly, which also avoids the extraminvalpass.Also, line 1131 is a leftover commented-out signature — please remove it.
🔧 Proposed fix: eliminate VLA with running minimum
function f_distance(boundary_v, boundary_edge_count, point) result(distance) $:GPU_ROUTINE(parallelism='[seq]') integer, intent(in) :: boundary_edge_count real(wp), intent(in), dimension(:, :, :) :: boundary_v - ! real(wp), intent(in), dimension(1:boundary_edge_count, 1:3, 1:2) :: boundary_v real(wp), dimension(1:3), intent(in) :: point integer :: i real(wp) :: dist_buffer1, dist_buffer2 - real(wp), dimension(1:boundary_edge_count) :: dist_buffer real(wp) :: distance - distance = 0._wp + distance = huge(1._wp) do i = 1, boundary_edge_count dist_buffer1 = sqrt((point(1) - boundary_v(i, 1, 1))**2 + & & (point(2) - boundary_v(i, 1, 2))**2) dist_buffer2 = sqrt((point(1) - boundary_v(i, 2, 1))**2 + & & (point(2) - boundary_v(i, 2, 2))**2) - dist_buffer(i) = minval((/dist_buffer1, dist_buffer2/)) + distance = min(distance, min(dist_buffer1, dist_buffer2)) end do - distance = minval(dist_buffer) - end function f_distance
1159-1194:⚠️ Potential issue | 🟡 MinorPotential out-of-bounds access when
boundary_edge_count == 0.If
boundary_edge_countis 0,idx_bufferstays at 0 (line 1174), and the access at line 1190 (boundary_v(idx_buffer, 3, 1)) is an out-of-bounds read. Since this is aGPU_ROUTINE(seq), it cannot calls_mpi_abort, but an early-return guard would prevent the OOB.🛡️ Proposed defensive guard
dist_buffer = 0._wp dist_min = initial_distance_buffer idx_buffer = 0 + if (boundary_edge_count == 0) then + normals = 0._wp + return + end if + do i = 1, boundary_edge_count
1060-1116:⚠️ Potential issue | 🟡 MinorSame OOB risk:
tri_idx = 0whenmodel%ntrs == 0.Lines 1111–1113 access
model%trs(tri_idx)%n(...)withtri_idxinitialized to 0 (line 1080). If the model has zero triangles, this is an out-of-bounds read. Same pattern asf_normalsabove.
🤖 Fix all issues with AI agents
In `@src/common/m_model.fpp`:
- Line 65: Remove the extra space between the allocate keyword and the opening
parenthesis in the allocation call so it matches the formatter expectation:
change the call to use allocate(model%trs(model%ntrs)) (referencing the allocate
invocation and the model%trs/model%ntrs symbols) — ensure no space exists
between "allocate" and "(".
In `@src/simulation/m_ib_patches.fpp`:
- Around line 216-220: The numeric literal 0.e5_wp is effectively zero and
misleading; in the grid padding assignments for grid_mm (when computing extents
from x_cc/y_cc/z_cc) replace the erroneous 0.e5_wp multiplier with the intended
half-cell padding 0.5_wp (e.g., use
0.5_wp*dx(0)/0.5_wp*dx(m)/0.5_wp*dy(0)/0.5_wp*dy(n)/0.5_wp*dz(0)/0.5_wp*dz(p) in
the grid_mm lines) — if you really intended zero padding instead, replace
0.e5_wp with the explicit 0._wp to make that clear.
🧹 Nitpick comments (1)
src/simulation/m_ib_patches.fpp (1)
487-489: Inconsistent allocation: plainallocateinstead of@:ALLOCATE.The 2D
s_ib_airfoil(lines 333–334) was updated to use@:ALLOCATEforairfoil_grid_uandairfoil_grid_l, but the 3Ds_ib_3D_airfoilstill uses plainallocate. This inconsistency means the 3D path may not properly allocate in GPU-managed memory. As per coding guidelines, use@:ALLOCATEfor device-aware allocation insrc/**/*.fpp.♻️ Proposed fix
if (.not. allocated(airfoil_grid_u)) then - allocate (airfoil_grid_u(1:Np)) - allocate (airfoil_grid_l(1:Np)) + @:ALLOCATE(airfoil_grid_u(1:Np)) + @:ALLOCATE(airfoil_grid_l(1:Np))
src/common/m_model.fpp
Outdated
| model%ntrs = nTriangles | ||
|
|
||
| allocate (model%trs(model%ntrs)) | ||
| allocate(model%trs(model%ntrs)) |
There was a problem hiding this comment.
Pipeline failure: allocate spacing.
The CI formatting check reports that this line has a space between allocate and (. Remove the space to match the expected format.
✏️ Suggested fix
- allocate (model%trs(model%ntrs))
+ allocate(model%trs(model%ntrs))🤖 Prompt for AI Agents
In `@src/common/m_model.fpp` at line 65, Remove the extra space between the
allocate keyword and the opening parenthesis in the allocation call so it
matches the formatter expectation: change the call to use
allocate(model%trs(model%ntrs)) (referencing the allocate invocation and the
model%trs/model%ntrs symbols) — ensure no space exists between "allocate" and
"(".
| grid_mm(1, :) = (/minval(x_cc(0:m)) - 0.e5_wp*dx(0), maxval(x_cc(0:m)) + 0.e5_wp*dx(m)/) | ||
| grid_mm(2, :) = (/minval(y_cc(0:n)) - 0.e5_wp*dy(0), maxval(y_cc(0:n)) + 0.e5_wp*dy(n)/) | ||
|
|
||
| if (p > 0) then | ||
| grid_mm(3, :) = (/minval(z_cc(0:p)) - 0.e5_wp*dz(0), maxval(z_cc(0:p)) + 0.e5_wp*dz(p)/) |
There was a problem hiding this comment.
0.e5_wp evaluates to 0.0 — likely a typo.
0.e5_wp in Fortran is 0.0 × 10^5 = 0.0, so the padding term 0.e5_wp*dx(0) is always zero. If the intent was to add half-cell padding for display, this should be 0.5_wp*dx(0). If zero padding is intentional, use 0._wp for clarity instead of the misleading 0.e5_wp.
🤖 Prompt for AI Agents
In `@src/simulation/m_ib_patches.fpp` around lines 216 - 220, The numeric literal
0.e5_wp is effectively zero and misleading; in the grid padding assignments for
grid_mm (when computing extents from x_cc/y_cc/z_cc) replace the erroneous
0.e5_wp multiplier with the intended half-cell padding 0.5_wp (e.g., use
0.5_wp*dx(0)/0.5_wp*dx(m)/0.5_wp*dy(0)/0.5_wp*dy(n)/0.5_wp*dz(0)/0.5_wp*dz(p) in
the grid_mm lines) — if you really intended zero padding instead, replace
0.e5_wp with the explicit 0._wp to make that clear.
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
|
||
| integer :: i, patch_id, patch_geometry | ||
|
|
||
| $:GPU_UPDATE(device='[gps(1:num_gps)]') |
There was a problem hiding this comment.
Suggestion: The explicit device update of the gps array before the GPU parallel loop is redundant with the copy='[gps]' clause on the GPU loop and, in typical accelerator models (e.g., OpenACC/OpenMP target), issuing an update for data that is not yet present on the device can produce a runtime "variable not present on device" error; removing this update and relying on the copy clause avoids this inconsistency. [possible bug]
Severity Level: Major ⚠️
- ❌ Levelset GPU path may abort at s_apply_levelset (src/...m_compute_levelset.fpp:29).
- ⚠️ GPU-accelerated runs of IB levelset computation unstable.
- ⚠️ Affects simulations using device backends (GPU/accelerator).| $:GPU_UPDATE(device='[gps(1:num_gps)]') | |
| ! gps data is transferred via the GPU_PARALLEL_LOOP copy clause; no explicit device update needed here |
Steps of Reproduction ✅
1. Execute code path that runs subroutine s_apply_levelset defined in
src/simulation/m_compute_levelset.fpp:29 (impure subroutine s_apply_levelset(gps,
num_gps)).
2. At runtime s_apply_levelset reaches the explicit device update on line 36:
"$:GPU_UPDATE(device='[gps(1:num_gps)]')".
3. Immediately after the update the code enters the GPU parallel region beginning at the
GPU_PARALLEL_LOOP on line 41 which contains copy='[gps]'. If the accelerator runtime does
not already have a device allocation for gps, the explicit GPU_UPDATE on line 36 can cause
the accelerator to error with "variable not present on device" (device update invoked
before device allocation).
4. Observable failure: execution abort or runtime accelerator error reported at or before
line 36 when running with GPU/accelerator enabled. If the explicit update is removed, the
gpu-parallel loop's copy clause at line 41 will allocate and transfer gps as needed and
avoid the pre-update error.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_compute_levelset.fpp
**Line:** 36:36
**Comment:**
*Possible Bug: The explicit device update of the `gps` array before the GPU parallel loop is redundant with the `copy='[gps]'` clause on the GPU loop and, in typical accelerator models (e.g., OpenACC/OpenMP target), issuing an update for data that is not yet present on the device can produce a runtime "variable not present on device" error; removing this update and relying on the `copy` clause avoids this inconsistency.
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.|
|
||
| end if | ||
|
|
||
| $:GPU_UPDATE(host='[gps(1:num_gps)]') |
There was a problem hiding this comment.
Suggestion: The explicit host update of the gps array after the GPU parallel loop is redundant with the copy='[gps]' clause on the GPU loop and, if the device copy created by the loop is deallocated at loop exit (as is typical), a subsequent host update for non-present data can raise a runtime error; removing this update ensures consistent device data lifetime. [possible bug]
Severity Level: Major ⚠️
- ❌ Post-levelset GPU-to-host transfer may abort simulation.
- ⚠️ Host-visible gps updates redundant with loop copy clause.
- ⚠️ Affects GPU-enabled runs of s_apply_levelset (src/...m_compute_levelset.fpp:29).| $:GPU_UPDATE(host='[gps(1:num_gps)]') | |
| ! gps data is already copied back to the host by the GPU_PARALLEL_LOOP copy clause; no explicit host update needed here |
Steps of Reproduction ✅
1. Run the same s_apply_levelset subroutine in src/simulation/m_compute_levelset.fpp:29
which executes a GPU_PARALLEL_LOOP (3D branch at ~line 41 or 2D branch at ~line 66) using
copy='[gps]'.
2. The GPU_PARALLEL_LOOP completes (END_GPU_PARALLEL_LOOP around lines 60/85) and typical
accelerator runtimes will deallocate per-loop device copies that were created by the
loop's copy clause.
3. Immediately after the loop the code executes the explicit host update at line 90:
"$:GPU_UPDATE(host='[gps(1:num_gps)]')". If the runtime has already released the device
allocation for gps, the explicit host update attempts to access a non-existent device copy
and may fail with "variable not present on device".
4. Observable failure: runtime error or abort reported at the host-update statement
(src/simulation/m_compute_levelset.fpp:90) on GPU-enabled runs. Relying on the loop's copy
clause avoids this double-management of gps memory.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/simulation/m_compute_levelset.fpp
**Line:** 90:90
**Comment:**
*Possible Bug: The explicit host update of the `gps` array after the GPU parallel loop is redundant with the `copy='[gps]'` clause on the GPU loop and, if the device copy created by the loop is deallocated at loop exit (as is typical), a subsequent host update for non-present data can raise a runtime error; removing this update ensures consistent device data lifetime.
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 Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/common/m_model.fpp (3)
1060-1116:⚠️ Potential issue | 🟡 MinorPotential out-of-bounds access when
model%ntrs == 0.With the
GPU_ROUTINEdirective now marking this subroutine as device-callable, the pre-existing edge case becomes more concerning: ifmodel%ntrsis 0, the loop on line 1081 is skipped,tri_idxremains 0 (line 1080), and lines 1111–1113 accessmodel%trs(0)%n, which is an out-of-bounds read. On a GPU this is undefined behavior with no clean abort path.🛡️ Proposed guard
+ if (tri_idx == 0) then + distance = 0._wp + normals = 0._wp + return + end if + normals(1) = model%trs(tri_idx)%n(1)
1159-1194:⚠️ Potential issue | 🟡 MinorSame zero-count out-of-bounds risk as
f_distance_normals_3D.If
boundary_edge_countis 0,idx_bufferstays 0 (line 1174) and lines 1190–1192 readboundary_v(0, …). Now that this is a GPU device routine, the undefined access is silent. Consider adding an early-return guard, mirroring the fix suggested forf_distance_normals_3D.
1125-1152:⚠️ Potential issue | 🟠 MajorVariable-length automatic array in GPU device code.
dist_buffer(1:boundary_edge_count)(line 1136) is a stack-allocated VLA inside aGPU_ROUTINE(seq)function. Device threads have very limited stack space; a largeboundary_edge_countwill overflow it. Moreover, some GPU compilers (nvfortran) may reject automatic arrays in device routines outright.The array is only used to collect per-edge minimums and then take
minval. Replace it with a scalar running minimum:♻️ Proposed refactor
function f_distance(boundary_v, boundary_edge_count, point) result(distance) $:GPU_ROUTINE(parallelism='[seq]') integer, intent(in) :: boundary_edge_count real(wp), intent(in), dimension(:, :, :) :: boundary_v real(wp), dimension(1:3), intent(in) :: point integer :: i real(wp) :: dist_buffer1, dist_buffer2 - real(wp), dimension(1:boundary_edge_count) :: dist_buffer real(wp) :: distance - distance = 0._wp + distance = initial_distance_buffer do i = 1, boundary_edge_count dist_buffer1 = sqrt((point(1) - boundary_v(i, 1, 1))**2 + & & (point(2) - boundary_v(i, 1, 2))**2) dist_buffer2 = sqrt((point(1) - boundary_v(i, 2, 1))**2 + & & (point(2) - boundary_v(i, 2, 2))**2) - dist_buffer(i) = minval((/dist_buffer1, dist_buffer2/)) + distance = min(distance, min(dist_buffer1, dist_buffer2)) end do - distance = minval(dist_buffer) - end function f_distanceDoes nvfortran support automatic arrays in OpenACC device routines?
🤖 Fix all issues with AI agents
In `@src/simulation/m_compute_levelset.fpp`:
- Around line 310-311: The Doxygen header above subroutine s_rectangle_levelset
is a copy-paste artifact ("Initialize IBM module") and should be replaced with a
concise description of what s_rectangle_levelset actually does: compute and
initialize a rectangular level-set (or populate level-set fields for a
rectangle), list key inputs/outputs (e.g., gp or grid/state being modified), and
any important assumptions; update the comment block associated with
s_rectangle_levelset accordingly so it accurately documents the rectangle
levelset computation and its parameters.
- Around line 643-648: The Doxygen `@param` tags are out-of-date for subroutine
s_model_levelset(gp): remove or replace the nonexistent parameters (patch_id,
STL_levelset, STL_levelset_norm) and instead document the actual argument gp and
its purpose (e.g., type/structure and role in the routine) in the comment header
above s_model_levelset; alternatively, if those other parameters are meant to be
inputs, update the subroutine signature to include them consistently—ensure the
docblock and the subroutine argument list match and use correct Doxygen `@param`
entries for gp (and any real parameters).
- Around line 558-559: The assignment to gp%levelset_norm uses an integer array
constructor (/1, 0, 0/) which relies on implicit conversion and doesn’t ensure
working precision; replace it with wp-suffixed real literals to match the module
precision (e.g., set gp%levelset_norm to a real(wp) array like (1._wp, 0._wp,
0._wp)) so the values are stored in the correct precision and consistent with
f_approx_equal and other wp-typed variables.
- Around line 708-714: The comments for the interpolate branches are swapped:
update the comment above the interpolate==.true. branch (where gp%levelset is
set via f_interpolated_distance using models(patch_id)%interpolated_boundary_v
and total_vertices) to say it is computing the shortest distance to the
interpolated model boundary, and update the comment above the
interpolate==.false. branch (where gp%levelset is set via f_distance using
models(patch_id)%boundary_v and boundary_edge_count) to say it is computing the
shortest distance to the actual model boundary; leave the code (interpolate,
gp%levelset, f_interpolated_distance, f_distance, xyz_local) unchanged.
- Line 419: Normalize normal_vector safely by guarding against division by zero:
compute the squared norm via dot_product(normal_vector, normal_vector), clamp it
with sgm_eps (e.g. use max(dot_product(normal_vector, normal_vector), sgm_eps)),
take the sqrt and divide by that clamped value instead of dividing by raw
sqrt(dot_product(...)); update the normalization at the end of the
m_compute_levelset routine where normal_vector is normalized to use this clamped
denominator (refer to normal_vector, dot_product and sgm_eps).
🧹 Nitpick comments (4)
src/common/m_model.fpp (3)
490-511: Commented-out GPU directive — consider implementing the fibonacci sphere now.The
GPU_ROUTINEdirective is commented out (line 490) while all other routines in this file (f_distance_normals_3D,f_distance,f_normals,f_interpolated_distance) have active directives. The TODO block (lines 504–511) correctly identifiesrandom_numberas the blocker and proposes a fibonacci sphere replacement.Since this PR's objective is to enable GPU-parallelized levelset computation, leaving
f_model_is_insideas the sole non-GPU-callable routine may limit STL model support on GPU. If the fibonacci sphere approach is validated, implementing it here would make the GPU story complete.
1130-1131: Remove commented-out old declaration.Line 1131 is a leftover from the previous fixed-shape signature. Dead commented-out code adds noise.
1-27: File exceeds 1000-line guideline (1251 lines).This is pre-existing, but with the new
m_compute_levelsetmodule being introduced in this PR, it may be a good time to evaluate whether some of the interpolation routines (f_interpolate_2D,f_interpolate_3D,f_check_interpolation_2D,f_check_interpolation_3D) could be split into a separate module to bring this file under the limit. As per coding guidelines: "Keep … files ≤ 1000 lines."src/simulation/m_compute_levelset.fpp (1)
598-613: Integer array constructors fordist_sides_vecanddist_surface_vec.Same
wpprecision concern as the sphere subroutine —(/1, 0, 0/),(/0, 1, 0/), etc. produce default-real values rather thanwp-precision. These are then used indot_productand arithmetic withreal(wp)variables.Proposed fix (representative — apply to all six occurrences)
- dist_sides_vec = (/1, 0, 0/) - dist_surface_vec = (/0, 1, 1/) + dist_sides_vec = (/1._wp, 0._wp, 0._wp/) + dist_surface_vec = (/0._wp, 1._wp, 1._wp/)As per coding guidelines: "Use
wp(working precision) parameter fromm_precision_selectinstead of hardcoded precision."
| !> Initialize IBM module | ||
| subroutine s_rectangle_levelset(gp) |
There was a problem hiding this comment.
Misleading Doxygen: "Initialize IBM module" on a rectangle levelset subroutine.
Copy-paste artifact. Should describe the rectangle levelset computation.
Proposed fix
- !> Initialize IBM module
+ !> Compute the levelset and normal for a rectangle patch
subroutine s_rectangle_levelset(gp)🤖 Prompt for AI Agents
In `@src/simulation/m_compute_levelset.fpp` around lines 310 - 311, The Doxygen
header above subroutine s_rectangle_levelset is a copy-paste artifact
("Initialize IBM module") and should be replaced with a concise description of
what s_rectangle_levelset actually does: compute and initialize a rectangular
level-set (or populate level-set fields for a rectangle), list key
inputs/outputs (e.g., gp or grid/state being modified), and any important
assumptions; update the comment block associated with s_rectangle_levelset
accordingly so it accurately documents the rectangle levelset computation and
its parameters.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/simulation/m_compute_levelset.fpp`:
- Around line 36-41: The GPU loops use copy='[gps]' while gps is already moved
to the device by GPU_UPDATE(device='[gps(1:num_gps)]'), causing
redundant/conflicting data regions; in the GPU_PARALLEL_LOOP pragmas (the loops
that list private='[i,patch_id,patch_geometry]', copy='[gps]',
copyin='[patch_ib,Np]') replace copy='[gps]' with present='[gps]' (and apply the
same change to the second loop around line 66) so the loops reference the
existing device-resident gps and rely on the GPU_UPDATE(host=...) copy-back
later.
- Around line 118-119: The assignment uses an integer literal 0 for the real(wp)
array element gp%levelset_norm; change the literal to the working precision kind
(0._wp) to avoid implicit conversion and match the fix in
s_sphere_levelset—update the assignment in the block where f_approx_equal(dist,
0._wp) is checked so it reads gp%levelset_norm = 0._wp (ensure _wp from
m_precision_select is used).
🧹 Nitpick comments (3)
src/simulation/m_compute_levelset.fpp (3)
156-197: Multiple integer literals assigned toreal(wp)variables.Lines 175, 181, 195 assign
0(integer) todist_vec(3)which isreal(wp). Use0._wpconsistently.Proposed fix (representative)
- dist_vec(3) = 0 + dist_vec(3) = 0._wpApply at lines 175, 181, and 195.
222-222: Unused variablelength_z.
length_zis declared but never referenced —lzis used instead. Remove it to avoid confusion.Proposed fix
- real(wp) :: length_z
598-613: Integer array constructors assigned toreal(wp)vectors.Lines 601–612 use
(/1, 0, 0/),(/0, 1, 0/), etc. fordist_sides_vecanddist_surface_vecwhich arereal(wp). Use_wpsuffix on all literals for type safety and precision consistency.Proposed fix (representative)
- dist_sides_vec = (/1, 0, 0/) - dist_surface_vec = (/0, 1, 1/) + dist_sides_vec = (/1._wp, 0._wp, 0._wp/) + dist_surface_vec = (/0._wp, 1._wp, 1._wp/)Apply similarly for lines 606–607 and 611–612.
| $:GPU_UPDATE(device='[gps(1:num_gps)]') | ||
|
|
||
| ! 3D Patch Geometries | ||
| if (p > 0) then | ||
|
|
||
| $:GPU_PARALLEL_LOOP(private='[i,patch_id,patch_geometry]', copy='[gps]', copyin='[patch_ib,Np]') |
There was a problem hiding this comment.
copy='[gps]' in both GPU loops conflicts with the GPU_UPDATE(device=...) at line 36.
Line 36 moves gps to the device via GPU_UPDATE. The subsequent copy='[gps]' in both loops (lines 41, 66) creates a separate structured data region that redundantly allocates/copies gps, potentially conflicting with the unstructured data update. Use present='[gps]' instead, since gps is already on the device, and rely on the GPU_UPDATE(host=...) at line 90 for the copy-back.
Proposed fix
- $:GPU_PARALLEL_LOOP(private='[i,patch_id,patch_geometry]', copy='[gps]', copyin='[patch_ib,Np]')
+ $:GPU_PARALLEL_LOOP(private='[i,patch_id,patch_geometry]', present='[gps]', copyin='[patch_ib,Np]')Apply similarly at line 66.
Also applies to: 64-66
🤖 Prompt for AI Agents
In `@src/simulation/m_compute_levelset.fpp` around lines 36 - 41, The GPU loops
use copy='[gps]' while gps is already moved to the device by
GPU_UPDATE(device='[gps(1:num_gps)]'), causing redundant/conflicting data
regions; in the GPU_PARALLEL_LOOP pragmas (the loops that list
private='[i,patch_id,patch_geometry]', copy='[gps]', copyin='[patch_ib,Np]')
replace copy='[gps]' with present='[gps]' (and apply the same change to the
second loop around line 66) so the loops reference the existing device-resident
gps and rely on the GPU_UPDATE(host=...) copy-back later.
| if (f_approx_equal(dist, 0._wp)) then | ||
| gp%levelset_norm = 0 |
There was a problem hiding this comment.
Integer literal 0 assigned to real(wp) array.
Same issue that was fixed for s_sphere_levelset at line 559 — use 0._wp for consistency and to avoid implicit conversion.
Proposed fix
- gp%levelset_norm = 0
+ gp%levelset_norm = 0._wpAs per coding guidelines: "Use wp (working precision) parameter from m_precision_select instead of hardcoded precision like real*8."
🤖 Prompt for AI Agents
In `@src/simulation/m_compute_levelset.fpp` around lines 118 - 119, The assignment
uses an integer literal 0 for the real(wp) array element gp%levelset_norm;
change the literal to the working precision kind (0._wp) to avoid implicit
conversion and match the fix in s_sphere_levelset—update the assignment in the
block where f_approx_equal(dist, 0._wp) is checked so it reads gp%levelset_norm
= 0._wp (ensure _wp from m_precision_select is used).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/simulation/m_ib_patches.fpp (1)
64-88:⚠️ Potential issue | 🟠 MajorRemove the
optionalattribute fromib_markers_sfparameter.Line 66 declares
ib_markers_sfasoptional, but it is used unconditionally throughout the subroutine and passed directly to geometry-specific subroutines (s_ib_sphere,s_ib_cuboid,s_ib_cylinder,s_ib_3D_airfoil,s_ib_model,s_ib_ellipse,s_ib_circle,s_ib_rectangle,s_ib_airfoil) that declare it as non-optionalintent(inout). Both call sites inm_ibm.fpp(lines 111 and 1002) always provide this argument. Remove theoptionalattribute to match actual usage and prevent potential undefined behavior.
🤖 Fix all issues with AI agents
In `@src/simulation/m_ib_patches.fpp`:
- Line 143: Replace the raw Fortran allocate call with the Fypp device-aware
macro: in the allocation of models(patch_id)%model (referencing the models
array, patch_id index and model component), remove the plain "allocate" and use
the Fypp macro invocation @:ALLOCATE(models(patch_id)%model) so the allocation
becomes device-aware and satisfies the pipeline formatting rule.
- Around line 58-60: The allocation for GPU-managed array models (declared with
GPU_DECLARE) must use the fypp device-aware macro rather than plain Fortran
allocate: replace the plain allocate of models(patch_id)%model with
@:ALLOCATE(models(patch_id)%model) so the memory is allocated on the device
(symbols: models, GPU_DECLARE, models(patch_id)%model, @:ALLOCATE). Also fix the
scalar concatenation bug by replacing constructs that form (/dx, dy, dz/) with
scalar minima using minval(dx), minval(dy), minval(dz) (addresses the
concatenation at the spots handling dx/dy/dz) and correct the typo 0.e5_wp to
0.e-5_wp where tolerance is intended (fix occurrences around the 215–219
region).
🧹 Nitpick comments (1)
src/simulation/m_ib_patches.fpp (1)
928-958: Serial triple-nested loop over the entire grid is a significant performance bottleneck.The TODO at line 928 explains that
f_model_is_insideusesrandom_number()which is non-GPU-compatible. Until resolved, this loop runs serially over all(m+1)×(n+1)×(p+1)cells, which will be very slow on large grids.Additionally, the loop order (
ioutermost,kinnermost at line 930–932) is suboptimal for Fortran's column-major layout withib_markers_sf(i, j, k). Consider swapping tok-j-iordering (k outermost, i innermost) to improve cache locality while this remains CPU-only.Suggested loop reorder for better cache performance
- do i = 0, m - do j = 0, n - do k = 0, p + do k = 0, p + do j = 0, n + do i = 0, m
| type(t_model_array), allocatable, target :: models(:) | ||
| $:GPU_DECLARE(create='[models]') | ||
| !! array of STL models that can be allocated and then used in IB marker and levelset compute |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP 'allocate\s*\(\s*models\s*\(' --type fortran -A2 -B2Repository: MFlowCode/MFC
Length of output: 39
🏁 Script executed:
rg -nP 's_instantiate_STL_models|@:ALLOCATE.*models' --type fortran -B3 -A3Repository: MFlowCode/MFC
Length of output: 39
🏁 Script executed:
rg -nP '\bmodels\s*=' --type fortran -B2 -A2 | head -100Repository: MFlowCode/MFC
Length of output: 39
🏁 Script executed:
cd src/simulation && sed -n '118,250p' m_ib_patches.fppRepository: MFlowCode/MFC
Length of output: 6421
🏁 Script executed:
rg -nP 'call\s+s_instantiate_STL_models|allocate\s*\(\s*models\s*\(' --type fortran -B3 -A3Repository: MFlowCode/MFC
Length of output: 39
🏁 Script executed:
rg -nP '@:ALLOCATE|allocate\s*\(\s*\w+\s*\(' src/simulation/ --type fortran -B1 -A1 | head -80Repository: MFlowCode/MFC
Length of output: 39
🏁 Script executed:
fd -e fpp -x grep -l '@:ALLOCATE\|allocate' {} \; src/simulation/ | head -5 | xargs -I{} sh -c 'echo "=== {} ===" && grep -n "@:ALLOCATE\|allocate" {} | head -10'Repository: MFlowCode/MFC
Length of output: 422
🏁 Script executed:
fd -e fpp . src/simulation/ | head -5 | xargs -I{} sh -c 'echo "=== {} ===" && grep -n "@:ALLOCATE" {} | head -5'Repository: MFlowCode/MFC
Length of output: 1335
🏁 Script executed:
sed -n '210,225p' src/simulation/m_ib_patches.fppRepository: MFlowCode/MFC
Length of output: 1064
🏁 Script executed:
sed -n '180,210p' src/simulation/m_ib_patches.fppRepository: MFlowCode/MFC
Length of output: 1499
Use @:ALLOCATE macro for device-aware allocation at line 143.
Line 143 allocates models(patch_id)%model using standard Fortran allocate(), but models is GPU-managed (declared with GPU_DECLARE at line 59). Per coding guidelines, use the fypp @:ALLOCATE() macro for all device-aware memory: @:ALLOCATE(models(patch_id)%model). This also resolves the pipeline formatting check failure.
Additionally, the prior review flagged two issues still present in the code:
- Lines 183, 200, 202:
(/dx, dy, dz/)concatenates allocatable arrays instead of scalars. Should useminval()(partially done at line 185 for 2D). - Lines 215–219:
0.e5_wpappears to be a typo for0.e-5_wp(current value is 0.0).
🤖 Prompt for AI Agents
In `@src/simulation/m_ib_patches.fpp` around lines 58 - 60, The allocation for
GPU-managed array models (declared with GPU_DECLARE) must use the fypp
device-aware macro rather than plain Fortran allocate: replace the plain
allocate of models(patch_id)%model with @:ALLOCATE(models(patch_id)%model) so
the memory is allocated on the device (symbols: models, GPU_DECLARE,
models(patch_id)%model, @:ALLOCATE). Also fix the scalar concatenation bug by
replacing constructs that form (/dx, dy, dz/) with scalar minima using
minval(dx), minval(dy), minval(dz) (addresses the concatenation at the spots
handling dx/dy/dz) and correct the typo 0.e5_wp to 0.e-5_wp where tolerance is
intended (fix occurrences around the 215–219 region).
src/simulation/m_ib_patches.fpp
Outdated
|
|
||
| do patch_id = 1, num_ibs | ||
| if (patch_ib(patch_id)%geometry == 5 .or. patch_ib(patch_id)%geometry == 12) then | ||
| allocate(models(patch_id)%model) |
There was a problem hiding this comment.
Use @:ALLOCATE macro for device-aware allocation.
Per coding guidelines, use the Fypp @:ALLOCATE macro instead of raw allocate. This also addresses the pipeline formatting failure flagged at this location.
Proposed fix
- allocate(models(patch_id)%model)
+ @:ALLOCATE(models(patch_id)%model)As per coding guidelines: "Use fypp macro @:ALLOCATE(var1, var2) for device-aware allocation instead of standard Fortran allocate"
🤖 Prompt for AI Agents
In `@src/simulation/m_ib_patches.fpp` at line 143, Replace the raw Fortran
allocate call with the Fypp device-aware macro: in the allocation of
models(patch_id)%model (referencing the models array, patch_id index and model
component), remove the plain "allocate" and use the Fypp macro invocation
@:ALLOCATE(models(patch_id)%model) so the allocation becomes device-aware and
satisfies the pipeline formatting rule.
…han on frontier. We should look into that eventually
|
@sbryngelson I ran the Phoenix ACC test suite manually and got it to pass, but every time I submit on the runnners I get this weird device error that I have never seen before. printed over and over before it just gets killed: Any idea what this could be? |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:XXL
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/simulation/m_ib_patches.fpp (1)
480-481: 🛠️ Refactor suggestion | 🟠 MajorUse
@:ALLOCATEforairfoil_grid_uandairfoil_grid_lin the 3D airfoil path.The 2D airfoil path (lines 325–326) correctly uses
@:ALLOCATE, but the 3D path still uses rawallocate. This inconsistency may cause issues if GPU-aware allocation behavior differs.Proposed fix
- allocate (airfoil_grid_u(1:Np)) - allocate (airfoil_grid_l(1:Np)) + @:ALLOCATE(airfoil_grid_u(1:Np)) + @:ALLOCATE(airfoil_grid_l(1:Np))As per coding guidelines: "Use fypp macro
@:ALLOCATE(var1, var2)for device-aware allocation instead of standard Fortranallocate."
🧹 Nitpick comments (4)
src/simulation/m_compute_levelset.fpp (3)
180-180: Bare integer0literals assigned toreal(wp)components throughout airfoil routines.Lines 180, 186, 200, 269, 275, and 289 assign
0(integer) todist_vec(3)(areal(wp)component). Use0._wpconsistently, as done on lines 119, 166, 255.As per coding guidelines: "Use
wp(working precision) parameter fromm_precision_selectinstead of hardcoded precision."
401-402: Unused variableskandidxdeclared but never referenced.These appear to be leftover from a different version of this subroutine (perhaps copied from
s_rectangle_levelset).
606-617: Integer array constructors fordist_sides_vecanddist_surface_vec.Lines 606–617 use
(/1, 0, 0/),(/0, 1, 0/), etc. forreal(wp)arrays. Usewp-suffixed literals for consistency.As per coding guidelines: "Use
wp(working precision) parameter fromm_precision_selectinstead of hardcoded precision."src/simulation/m_ib_patches.fpp (1)
907-908: Unused variablespointandlocal_pointins_ib_model.These variables are declared but never referenced in the subroutine body. They appear to be remnants from a prior version of the code.
Proposed fix
real(wp) :: eta - real(wp), dimension(1:3) :: point, local_point real(wp), dimension(1:3) :: center, xyz_local
|
CodeAnt AI Incremental review completed. |
1 participant
User description
User description
Description
This PR is a significant refactor of the levelset code. It contains
Fixes #1011
Type of change
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?
This currently passes all tests on MacOS and Ubuntu operating systms using the GNU compiler.
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:
nvtxranges so that they can be identified in profiles./mfc.sh run XXXX --gpu -t simulation --nsys, and have attached the output file (.nsys-rep) and plain text results to this PR./mfc.sh run XXXX --gpu -t simulation --rsys --hip-trace, and have attached the output file and plain text results to this PR.PR Type
Enhancement, Refactor
Description
Removed levelset computation from preprocessing: IB markers and levelset values are no longer computed during pre-processing; all computations now occur during simulation runtime
Eliminated global levelset arrays: Replaced global
levelsetandlevelset_normarrays with local storage in theghost_pointderived type, reducing memory allocation by orders of magnitudeParallelized levelset computation over ghost points: Refactored levelset code to parallelize over ghost points instead of grid cells, improving memory efficiency and GPU performance
GPU-accelerated STL model processing: Implemented GPU compute for STL IB markers and levelset values by loading STL models into global memory with separate parallelized loops
New
m_compute_levelsetmodule: Created dedicated module for all levelset computations with geometry-specific functions (circles, rectangles, ellipses, spheres, cylinders, airfoils, STL models)Added STL model configuration parameters: Extended IB patch parameters to support
model_filepath,model_spc,model_threshold, and transformation parameters (model_translate,model_rotate)Simplified data I/O: Removed levelset and levelset norm from preprocessing output; added dedicated IB data output subroutines for simulation runtime
Updated test infrastructure: Adjusted grid resolution for circle test cases and updated golden test files to reflect removal of pre-computed IB markers
Diagram Walkthrough
File Walkthrough
8 files
m_ib_patches.fpp
Refactor levelset computation from preprocessing to simulation runtimesrc/simulation/m_ib_patches.fpp
m_compute_levelsetmodule dependency and levelset computationcalls from
s_apply_ib_patchess_instantiate_STL_models()subroutine to load and preprocessSTL models during initialization
s_ib_model()to use pre-instantiated models from globalmodelsarray instead of computing levelsets inlineetavariable declarations and replaced with local scopeusage in geometry-specific functions
offsetvariable to airfoil subroutines for cleaner centroidoffset handling
m_data_output.fpp
Remove levelset data output from preprocessingsrc/pre_process/m_data_output.fpp
ib_markers,levelset, andlevelset_normparameters from dataoutput subroutine signatures
levelset norm data
s_write_abstract_data_files()m_ibm.fpp
Refactor IBM module to use ghost-point-based levelsetssrc/simulation/m_ibm.fpp
levelsetandlevelset_normarrays and replaced withghost-point-local storage
s_instantiate_STL_models()during IBM setups_apply_ib_patches()calls to remove levelset parameterss_compute_image_points()to use levelset values fromghost_pointstructure instead of global arrayss_update_mib()to call news_apply_levelset()instead ofcomputing levelsets inline
patch_id_fparray allocationm_start_up.fpp
Remove levelset restart file I/O from simulation startupsrc/simulation/m_start_up.fpp
from restart files
s_write_ib_data_file()after IBM setup duringinitialization
s_read_ic_data_files()call signature by removing IB markerparameter
m_start_up.fpp
Remove levelset handling from preprocessing startupsrc/pre_process/m_start_up.fpp
m_ib_patchesmodule importib_markers,levelset, andlevelset_normvariable declarationsand allocations
s_read_ic_data_files()interface by removing IB markerparameter
m_mpi_common.fpp
Simplify MPI data initialization for IBMsrc/common/m_mpi_common.fpp
levelsetandlevelset_normparameters froms_initialize_mpi_data()subroutinestructures
m_initial_condition.fpp
Remove IBM-related variables from preprocessingsrc/pre_process/m_initial_condition.fpp
m_ib_patchesmodule importib_markers,levelset, andlevelset_normvariable declarationss_apply_ib_patches()call from preprocessingm_time_steppers.fpp
Remove levelset parameters from immersed boundary updatesrc/simulation/m_time_steppers.fpp
s_update_mibsubroutine call by removinglevelsetandlevelset_normparameterssimulation rather than passed as arguments
5 files
m_compute_levelset.fpp
New module for ghost-point-based levelset computationsrc/simulation/m_compute_levelset.fpp
boundaries
s_apply_levelset()subroutine that computes levelsets forghost points instead of grid cells
ellipses, spheres, cylinders, airfoils, and STL models
ghost_pointderived type instead of global arrays
efficiency
m_data_output.fpp
Add dedicated IB data output subroutinessrc/simulation/m_data_output.fpp
s_write_serial_ib_data()ands_write_parallel_ib_data()for IB data outputs_write_ib_data_file()wrapper to handle both serial andparallel I/O
routines
parameters
m_global_parameters.fpp
Add default initialization for IB patch parameterssrc/simulation/m_global_parameters.fpp
patch_ib()array with default values forall IB patch parameters
translate, rotate)
m_derived_types.fpp
Add derived types for model storage and ghost point levelsetssrc/common/m_derived_types.fpp
t_model_arrayderived type to store STL model data withboundary vertices and interpolation information
ghost_pointderived type withlevelsetandlevelset_normfields for local storage
point structure
case_dicts.py
Add STL model and transformation parameters to IB configurationtoolchain/mfc/run/case_dicts.py
model_filepath,model_spc, andmodel_thresholdmodel_translateandmodel_rotatefor each spatial directionmodel_scaleparameter for potential future use7 files
cases.py
Adjust grid resolution for circle test casestoolchain/mfc/test/cases.py
n=49grid resolution for improvedprecision
circular geometries
golden.txt
Remove IB markers from golden test outputtests/7FA04E95/golden.txt
ib_markersoutput line from golden test dataand stored globally
golden.txt
Remove IB markers output from test golden filetests/5600D63B/golden.txt
D/ib_markers.00.datwith all zerovalues
D/cons.5.00.000050.datline with updated valuesgolden-metadata.txt
Update test metadata with new build environment detailstests/7F70E665/golden-metadata.txt
OpenMP : OFFconfiguration linegolden-metadata.txt
Update test metadata with new build environment detailstests/F60D6594/golden-metadata.txt
OpenMP : OFFconfiguration linegolden-metadata.txt
Update test metadata with new build environment detailstests/4F5A5E32/golden-metadata.txt
OpenMP : OFFconfiguration linegolden-metadata.txt
Update test metadata with new build environment detailstests/8D8F6424/golden-metadata.txt
OpenMP : OFFconfiguration line3 files
golden-metadata.txt
Update golden test metadata and build environmenttests/B0CE19C5/golden-metadata.txt
hash
paths, system information)
golden-metadata.txt
Update golden test metadata and build environmenttests/7DCE34B4/golden-metadata.txt
hash
paths, system information)
golden-metadata.txt
Update golden test metadata and build environmenttests/6171E9D4/golden-metadata.txt
hash
paths, system information)
specifications
63 files
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
CodeAnt-AI Description
Refactor levelset and STL immersed-boundary handling to compute values per ghost point
What Changed
Impact
✅ Lower memory during simulation✅ Support for rotated STL immersed boundaries✅ IB marker files available at startup and restart💡 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.