OpenMP element coloring + conditional velocity gather skip - #14
Open
mohitt31 wants to merge 4 commits into
Open
Conversation
Ports the element coloring from feature/batched-force-apply (trimmed, drops the batch_const/VEC_W batching machinery that branch needed for AVX2 but this doesn't) and adds an OPT_OMP build path that runs each color's elements across threads with the original scalar MAT_Fint, no new kernel. Correctness holds at every thread count tested, round off level diffs against the unflagged baseline, identical regardless of thread count since the reordering comes from the coloring, not from thread scheduling. Speedup is real but caps around 1.67x at 4 threads and doesn't improve past that on this machine's 10 cores, details and the honest read on why in README_OPENMP_COLORING.md.
Second full pass reproduces the plateau (1.7x/1.66x/1.62x at 4/8/10 threads, within a couple percent of the first run, correctness diffs bit for bit identical). Also built a diagnostic variant that skips the parallel region for the two tiny colors (75 and 2 elements) to test whether fork-join overhead on those was capping the scaling. It wasn't, the threshold run landed inside the same noise band as the untouched build at every thread count, so that hypothesis is out and the 4 performance core count looks like the real explanation. Didn't keep the threshold change since it tested negative.
…filer Adds DEFAULT(NONE) to the compute_Fint parallel region so every variable has to be classified explicitly, which turns an accidental shared write into a compile error instead of a race. Also adds an OPT_FINT_PROFILE build flag that wraps compute_Fint and the time loop in wall-clock timers, so the parallelizable fraction can be measured directly rather than inferred. Measured it at 0.689 (compute_Fint is 68.9% of the loop), matching the earlier profiling estimate, and with that fraction the observed thread scaling tracks Amdahl's law: the plateau at 4 threads is the serial-fraction ceiling plus the M4 only having 4 performance cores, not a bug. README updated with the measurement and the Amdahl check.
Profiled the gather/scatter, which the earlier work flagged as the next bottleneck. The data-layout transpose (npoin,ndof)->(ndof,npoin) that looked promising turns out to be worth only about 1 to 2% when measured with a microbenchmark on the real ibool, and it would touch field access everywhere, so it is not worth the churn. The real waste is that compute_Fint gathers both d and v every element every timestep, but MAT_Fint only reads v on the Kelvin-Voigt path, so for elastic (and any non Kelvin-Voigt material) the v gather is pure overhead. Added MAT_needs_veloc in mat_gen next to MAT_Fint to answer whether a material reads v, and compute_Fint now gathers v only when it does, in both the serial and OpenMP paths. On 2.5D_inplane this drops compute_Fint by 12.7% (about 9% end to end) with bit for bit identical seismograms, since v was dead for that material. Full analysis and the microbenchmark are in GATHER_SCATTER_ANALYSIS.md and gs_bench.f90.
jpampuero
reviewed
Aug 22, 2026
jpampuero
left a comment
jpampuero
left a comment
Owner
There was a problem hiding this comment.
Great work. I have only minor suggestions.
| do j = 1, ngll | ||
| do i = 1, ngll | ||
| p = ibool(i, j, e) | ||
| if (p >= 1 .and. p <= npoin) then |
Owner
There was a problem hiding this comment.
Is this check really necessary? (also below)
| enddo | ||
|
|
||
| ! Clear node_owner for this color | ||
| do idx = 1, coloring%colors(icol)%nelem |
Owner
There was a problem hiding this comment.
Can this loop be replaced by simply "node_owner = 0" ?
| do e = 1, nelem | ||
| do j = 1, ngll | ||
| do i = 1, ngll | ||
| p = ibool(i, j, e) |
Owner
There was a problem hiding this comment.
It would be more efficient to apply the greedy coloring algorithm to knods instead of ibool, because knods is much smaller: it's based on the element control nodes (4 to 9 per element) instead of the GLL nodes (ngll*ngll per element)
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
2 participants
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.
Two changes to compute_Fint, both on the 2.5D_inplane case (12800 elements, ngll 5):
1. OpenMP parallelism via element coloring
2. Conditional velocity gather
MAT_needs_veloc(matpro)query in mat_gen to gate the gatherGather/scatter analysis