AeroDyn OLAF performance optimization - #3464
Merged
Merged
Conversation
Hoist RegFunction select outside the CP/particle loops and add masked/branchless SIMD variants for idRegNone and idRegCompact. Use r2s=max(r2,MINNORM2) floor to keep the divide finite (no Inf/NaN), with an msk gate for near-coincident points. idRegExp keeps the guarded r>2rc branch that skips the exp (exp blocks SIMD anyway). Scalar ui_part_nograd_11 kernel is unchanged. Co-authored-by: Copilot <[email protected]> Co-authored-by: Claude <[email protected]>
Turn childless nodes into standard treecode leaves that carry moments and are MAC-tested, so a far control point uses the leaf multipole instead of direct- summing the bucket. Move early-leaf termination after the P2M moment loop so early leaves keep valid moments, and set the single-particle root monopole so a far MAC hit there is exact. Unify the traversal: every non-empty node computes distDirect/r, near -> direct-sum its leaves and recurse branches, far -> node multipole (held short of M2M). Add contained ui_leaves_nograd with masked/ branchless !$OMP SIMD over particles for idRegNone/idRegCompact (idRegExp left scalar). Early termination triggers on a small bucket (nPart<=Ncrit) or a sub- core cell, sizing leaves for the vector loop. Co-authored-by: Copilot <[email protected]> Co-authored-by: Claude <[email protected]>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical numerical-correctness and input-compatibility issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
modules/aerodyn/src/FVW_BiotSavart.f90:489
- The same
r>2*rcreplacement changes the scalarui_part_nograd_11kernel: for points just beyond the cutoff, it changes the factor from approximately1-exp(-8)to 1. This routine is also the reference path used by callers and tests, so the PR's claimed exponential-kernel equivalence is not preserved. Keep the original exponential expression unless this numerical approximation is intentional and documented.
if (r3 > PART_REG_CUT3*rc3) then ! r > 2*rc: mollifier -> 1 (skip exp), consistent with far-field multipole floor
ScalarPart = r3_inv*fourpi_inv
else
E = exp(-r3/rc3)
ScalarPart = (1._ReKi-E)*r3_inv*fourpi_inv
modules/aerodyn/src/FVW_BiotSavart.f90:441
- The compact SIMD path computes
tt = r2s/rc2and the polynomial branch unconditionally beforemerge.FVW_IOrejects only negative regularization parameters, so a zero core can reach this code withrc2=0and execute division by zero; the scalarui_part_nograd_11avoids that by selecting the outside-core branch first. Either reject zero cores forRegFunctionPart=2or make this arithmetic safe for a zero core.
tt = r2s/rc2
Cx = Alpha(2,ip)*dz - Alpha(3,ip)*dy
Cy = Alpha(3,ip)*dx - Alpha(1,ip)*dz
Cz = Alpha(1,ip)*dy - Alpha(2,ip)*dx
sp = merge(fourpi_inv/r3, (35._ReKi + tt*(-42._ReKi + 15._ReKi*tt))*fourpi_inv/(8._ReKi*rc3), r2s >= rc2)
modules/aerodyn/src/FVW_VortexTools.f90:1696
- The leaf kernel repeats the same hard cutoff, so tree-accelerated direct leaf interactions also differ from the prior exponential kernel by about
exp(-8)immediately beyond the boundary. This is a behavioral change beyond the stated SIMD/data-flow rewrite; either preserve the exponential expression or update the numerical-impact description and regression criteria.
if (r3 > PART_REG_CUT3*rc3) then ! r>2rc: mollifier->1, skip the expensive exp
sp = fourpi_inv/r3
else
sp = (1.0_ReKi-exp(-r3/rc3))*fourpi_inv/r3
end if
modules/aerodyn/src/FVW_VortexTools.f90:1710
- The leaf SIMD kernel has the same zero-core problem: for the accepted
RegParam == 0input,rc2andrc3are zero andtt = r2s/rc2performs invalid arithmetic for every particle beforeMERGEselects the singular result. This should share the zero-core handling withui_part_nogradso tree and direct particle methods remain valid under the same input.
rc2 = (PART_REG_C2*Part%RegParam(ip))**2
rc3 = rc2*PART_REG_C2*Part%RegParam(ip)
tt = r2s/rc2
- Files reviewed: 14/14 changed files
- Comments generated: 4
- Review effort level: Lite
The multipole expansion only converges when the target lies outside the source radius (BranchFactor >= 1); a value < 1 makes the series diverge. This was previously unchecked, so any value (including negative) was accepted. Enforce the bound at input time, only when a tree velocity method is selected. Co-authored-by: Copilot <[email protected]> Co-authored-by: Claude <[email protected]>
luwang00
marked this pull request as ready for review
September 16, 2026 19:57
andrew-platt
approved these changes
Sep 25, 2026
…idation, segment-tree invariant note C1: floor the compact-C2 divisors (rc2/rc3) and clamp tt in the branchless idRegCompact SIMD kernels (ui_part_nograd, ui_leaves_nograd) so the always-evaluated, discarded merge arm never divides by zero or overflows when RegParam==0. Behavior-preserving. C2: validate TreeBranchFactor for both VelocityMethod indices (loop over 1:2) and document the >=1 requirement in the OLAF input-file docs. C3: document the segment-tree invariant in ui_tree_segment_11 (comment only). Co-authored-by: GitHub Copilot <[email protected]> Co-authored-by: Claude <[email protected]>
gfortran's default -ftrapping-math blocks if-conversion of the branchless compact-C2 merge in the OLAF vortex kernels, preventing vectorization. The C1 divisor floors make disabling trapping-math safe (discarded merge arm can't divide by zero). Scoped to FVW_BiotSavart.f90 and FVW_VortexTools.f90, and skipped when FPE_TRAP_ENABLED so trap-checking builds keep meaningful FPE traps. Co-authored-by: GitHub Copilot <[email protected]> Co-authored-by: Claude <[email protected]>
The treecode leaf kernel (ui_leaves_nograd) read sources via ip=leaves(i), an indirect stride-3 gather that blocked SIMD (idRegNone leaf failed with 'no vectype for stmt' on the SSE2 baseline). A post-build O(n) pass (compact_tree_part) copies each node's leaf particles into a contiguous, column-major Tree%LeafBuf(nAct,7) [Px Py Pz Ax Ay Az Rc], and each node records its bucket slice via iLeafStart/iLeafN. The kernel now reads unit-stride columns -> packed loads. Result-neutral (scratch copy; the shared Part arrays and the control points are untouched). Empirically both vectorizable leaf loops now vectorize (16-byte): idRegNone (previously gather-blocked) and idRegCompact; idRegExp stays scalar by design (exp control flow). All OLAF regression tests pass within tolerance. Co-authored-by: GitHub Copilot <[email protected]> Co-authored-by: Claude <[email protected]>
…FLAGS FPE_TRAP_ENABLED only toggles the -DFPE_TRAP_ENABLED define, not any -ffpe-trap compile flag, so a user enabling traps via -DCMAKE_Fortran_FLAGS=-ffpe-trap=... would still get -fno-trapping-math on the FVW kernels. Guard on that flag too. Co-authored-by: GitHub Copilot <[email protected]> Co-authored-by: Claude <[email protected]>
As a contained procedure of ui_tree_part, the leaf kernel host-associated Tree%LeafBuf; under -fopenmp the enclosing PARALLEL DO outlining lost the descriptor's unit-stride proof and degraded the packed loads to a gather, so the None/Compact leaf loops did not vectorize in an OpenMP build (the default VelocityMethod path). Passing LeafBuf as an explicit contiguous dummy re-establishes unit stride in the callee independent of caller outlining; both leaf loops now vectorize 16B under OPENMP=ON, with no change under OPENMP=OFF. Co-authored-by: GitHub Copilot <[email protected]> Co-authored-by: Claude <[email protected]>
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.
This PR is ready to be merged once
Feature or improvement description
Speeds up the OLAF free-wake near-field particle-to-particle sum by
Also added input validation for
TreeBranchFactorrequiring it to be >= 1 (default is 1.5) for multipole expansion convergence. (Note that the exact threshold for convergence depends on the particle distribution within a node with a worst case requirement of sqrt(3)=1.73, so taking >=1 as a reasonable requirement here.)No new inputs; this is purely a data-flow/structure rework.
Based on one production case, the overall simulation time reduced by 41% from 2.8 days to 1.7 days.
Branch-free, SIMD-friendly near-field kernels
Rewrites the batch direct sum (
ui_part_nograd) and adds a contained tree-leaf kernel (ui_leaves_nograd) in masked/branchless form with!$OMP SIMD reductionfor the inner loop over source particles. (This is nested within the!$OMP PARALLELouter loop over the query points.)The singularity guard (cycle) is replaced by a mask (
msk = merge(1,0,r²≥MINNORM²)) plus a floored divisor (r2s = max(r², MINNORM²)) so the divide is always finite (noInf/NaN) and the loop has no data-dependent control flow. TheRegFunctionselect case is hoisted out of the loops over particles so each inner loop is uniform. This lets the None and Compact branches vectorize; the Exponential branch is deliberately left scalar (itsexp()blocks SIMD), so the Exponential kernel arithmetic itself is unchanged. (Overall Exponential kernel results still shift due to the leaf-multipole/MAC traversal change and tree branching early termination described below.)Treecode leaf multipoles + Multipole Acceptance Criterion (MAC) on leaves + early tree branching termination
Childless nodes are now standard treecode leaves that carry multipole moments and are MAC-tested: a far query/control point uses the leaf's multipole instead of direct-summing the bucket, while a near query point direct-sums the leaf's particles via the SIMD leaf kernel.
Early termination kicks in when a node's radius drops below the largest core size in the cell (
<=maxRegParam, i.e., a sub-core cell) or its particle count falls below the bucket cap (currently hardcoded to 32 as an SIMD friendly size). The first clause bounds tree depth in dense clumps; the second helps the inner loop vectorize.These changes also simplify the tree traversal: childless nodes are no longer a force-direct special case but follow the same MAC path as internal nodes, and leaf buckets now carry multipole moments accepted when well-separated. This brings the implementation closer to a textbook treecode.
Note on terminology: This codebase uses "leaves" in a nonstandard way. It refers to the particles belonging to a node. In standard treecode terminology, leaves are the lowest-level (childless) nodes. This PR message uses the standard terminology: "leaf" = childless node, and "the leaf's particles/bucket" for the particles it holds.
Related issue, if one exists
Requires the compact polynomial core added through PR #3457.
Impacted areas of the software
FVW_BiotSavart.f90masked/branchless SIMD rewrite of the batchui_part_nograddirect sum.FVW_VortexTools.f90SIMD leaf kernelui_leaves_nograd, leaf multipoles + MAC-on-leaves, early branching terminationNo input-file, registry, or API changes.
Additional supporting information
The SIMD payoff requires a vectorizable regularization kernel (None or Compact). With the default Exponential kernel, the leaf loop stays scalar, so this PR yields little to no speedup. It pairs with the compact-C2 core from the earlier PR #3457, which removes the
exp()and makes the near-field loop vectorizable. Considering the vast improvement in performance, we might want to make the compact-C2 core default in the future.Numerical impact: SIMD reduction reordering changes results at floating-point truncation error level. The leaf-multipole/MAC change makes far childless-node interactions multipole-approximated rather than exact-direct, bounded by the existing
BranchFactoracceptance criterion. Both are within regression tolerance, but not bit-for-bit. The early tree branching termination also alters the tree structure by reducing the depth of the tree.With this optimization, we held short of M2M upward path for future work. M2M reduces multipole moment construction from O(NlogN) of direct/pure P2M to O(N), but it is not the bottleneck at the moment. Similarly, additional steps like M2L, L2L, and L2P for a full O(N) fast-multipole implementation are likely not helpful right now due to nearfield P2P dominance. These help only once far-field aggregation becomes a significant fraction (typically large N on the order of 10⁶). (P: Particle, M: Multipole expansion, L: Local expansion)
Test results, if applicable
Performance (multi-day production case with compact kernel): 41% wall-clock reduction (1.69×) from the SIMD near-field sum alone. Combining both SIMD vectorization from this PR and the Compact-C2 core from PR #3457, total speed up relative to the original OLAF with exponential cores is just over 3x on the same hardware.
Regression: matches within r-test tolerance.
Generative AI usage
Co-authored-by: Anthropic Claude [email protected]
Assisted by: GitHub Copilot [email protected]