Skip to content

Commit 4ccf31f

Browse files
imreddyTejaclaude
andcommitted
Share one foreach_axis/sum_axes pair between the CPU and GPU operators
The axis loop was written two ways: the CPU operators called `unrolled_foreach(axis_vals(op))` inline at five sites, each repeating the `@inline` annotation and a comment explaining it, while the CUDA extension had its own `sum_over_axes` plus a bare `unrolled_foreach(axis_vals(op))` in the shared-memory fills. Both now go through `foreach_axis` and `sum_axes`, defined once next to `axis_vals`, so the axis-tuple plumbing and the reason the per-axis accumulators are kept separate are stated in one place. The `@inline` stays in each closure body rather than moving into the helpers: only a method, not an anonymous function, can carry it, and forcing the inline from inside the helper -- by wrapping `f` in another closure to annotate its call site -- reintroduces the per-slab heap allocation it exists to prevent (I measured every operator kernel allocating again). The helpers' docstring now carries that explanation, along with the matching note about `@inbounds`. Verified: CPU results bitwise unchanged (74/74 expressions) with allocations back at zero for all 14 benchmark kernels; GPU results bitwise identical for all 70 supported expressions. Tests pass on both devices. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0148rCSSVswyipJTZjqpEUMd
1 parent 8804cf0 commit 4ccf31f

3 files changed

Lines changed: 59 additions & 28 deletions

File tree

ext/cuda/operators_sem_shmem.jl

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import ClimaCore.Operators: form_jacobian, form_weighted_arg
77
import ClimaCore.Operators:
88
axis_vals,
99
axis_index,
10+
foreach_axis,
1011
contravariant,
1112
curl_covariant_components,
1213
curl_uses_component,
@@ -91,7 +92,8 @@ Base.@propagate_inbounds function operator_fill_shmem!(
9192
local_geometry = get_local_geometry(space, ij, slabidx)
9293
node = shmem_index(op, ij)
9394
jacobian = form_jacobian(F(), local_geometry)
94-
unrolled_foreach(axis_vals(op)) do vd
95+
foreach_axis(op) do vd
96+
@inline
9597
@inbounds Jv[axis_index(vd)][node] =
9698
jacobian * contravariant(vd, arg, local_geometry)
9799
end
@@ -124,7 +126,8 @@ Base.@propagate_inbounds function operator_fill_shmem!(
124126
local_geometry = get_local_geometry(space, ij, slabidx)
125127
node = shmem_index(op, ij)
126128
(; J) = local_geometry
127-
unrolled_foreach(axis_vals(op)) do vd
129+
foreach_axis(op) do vd
130+
@inline
128131
@inbounds work[axis_index(vd)][node] =
129132
J * contravariant(vd, arg1, local_geometry)
130133
end

ext/cuda/operators_spectral_element.jl

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,9 @@ import ClimaCore.Operators:
1414
covariant_components,
1515
covariant_vector,
1616
curl_term,
17-
replace_index
18-
import UnrolledUtilities: unrolled_map, unrolled_reduce
17+
replace_index,
18+
sum_axes
19+
import UnrolledUtilities: unrolled_map
1920
import Base.Broadcast: Broadcasted
2021

2122
"""
@@ -194,9 +195,8 @@ end
194195
# form_deriv_entry supplies the derivative matrix entries (transposed and
195196
# sign-flipped for the weak form), form_jacobian_rescale or form_weight_rescale
196197
# divides the form's Jacobian or quadrature-weight factor back out of the result,
197-
# and the loop over axes is unrolled with one accumulator per dimension, combined
198-
# once at the end, which keeps the accumulation loops as independent dependency
199-
# chains.
198+
# and `sum_axes` unrolls the loop over axes, keeping one accumulator per
199+
# dimension.
200200

201201
"""
202202
apply_stencil(form, D, w, node, ::Val{d}, i, Nq)
@@ -242,9 +242,6 @@ end
242242
Base.@propagate_inbounds shmem_components(work, node) =
243243
unrolled_map(w_k -> isnothing(w_k) ? nothing : (@inbounds w_k[node]), work)
244244

245-
# Sum of the per-axis contributions of an operator, unrolled over its axes.
246-
@inline sum_over_axes(f, op) = unrolled_reduce(+, unrolled_map(f, axis_vals(op)))
247-
248245
Base.@propagate_inbounds function operator_evaluate(
249246
op::Divergence{I, F},
250247
Jv,
@@ -260,7 +257,8 @@ Base.@propagate_inbounds function operator_evaluate(
260257
local_geometry = get_local_geometry(space, ij, slabidx)
261258
node = shmem_index(op, ij)
262259

263-
DJv = sum_over_axes(op) do vd
260+
DJv = sum_axes(op) do vd
261+
@inline
264262
d = axis_index(vd)
265263
@inbounds apply_stencil(F(), D, Jv[d], node, vd, ij[d], Nq)
266264
end
@@ -283,7 +281,8 @@ Base.@propagate_inbounds function operator_evaluate(
283281
node = shmem_index(op, ij)
284282
psi = last(work)
285283

286-
result = sum_over_axes(op) do vd
284+
result = sum_axes(op) do vd
285+
@inline
287286
d = axis_index(vd)
288287
i = ij[d]
289288
Juᵈ = work[d]
@@ -347,7 +346,8 @@ Base.@propagate_inbounds function operator_evaluate(
347346
local_geometry = get_local_geometry(space, ij, slabidx)
348347
node = shmem_index(op, ij)
349348

350-
result = sum_over_axes(op) do vd
349+
result = sum_axes(op) do vd
350+
@inline
351351
d = axis_index(vd)
352352
@inbounds curl_stencil(F(), D, work, node, vd, ij[d], Nq)
353353
end

src/Operators/spectralelement.jl

Lines changed: 43 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
import UnrolledUtilities: unrolled_map, unrolled_foreach, unrolled_any
1+
import UnrolledUtilities:
2+
unrolled_map, unrolled_foreach, unrolled_reduce, unrolled_any
23
import ..Utilities.Unrolled: unrolled_map_with_inbounds
34

45
abstract type AbstractSpectralStyle <: Fields.AbstractFieldStyle end
@@ -670,10 +671,11 @@ Base.Broadcast.BroadcastStyle(
670671
# axis index in the type domain so that the loop over axes unrolls and the node
671672
# indices stay statically sized.
672673
#
673-
# The per-axis closures below carry two annotations that matter for performance:
674-
# `@inline`, because the mutable slab temporary they write to would otherwise
675-
# escape and be heap-allocated once per slab, and `@inbounds`, which a closure
676-
# body does not inherit from the enclosing `@inbounds` block.
674+
# Loops over axes go through `foreach_axis` and `sum_axes`, which own the
675+
# inlining discipline the per-axis closures depend on. Their bodies still need
676+
# their own `@inbounds`: a closure body does not inherit it from the enclosing
677+
# `@inbounds` block, and only a method (not an anonymous function) can opt into
678+
# propagating it.
677679

678680
"""
679681
axis_vals(op)
@@ -691,6 +693,32 @@ The axis index `d` itself: `ij[axis_index(vd)]` is the node index along axis `d`
691693
"""
692694
@inline axis_index(::Val{d}) where {d} = d
693695

696+
"""
697+
foreach_axis(f, op)
698+
sum_axes(f, op)
699+
700+
Call `f(Val(d))` for each axis index `d` that `op` works over, unrolled;
701+
`sum_axes` sums the results, keeping one accumulator per axis and combining them
702+
once at the end so that the per-axis accumulation loops stay independent
703+
dependency chains.
704+
705+
Two annotations belong in the body of `f`, and neither can be supplied from here,
706+
because only a method -- not an anonymous function -- can carry them:
707+
708+
- `@inline`, as the first statement. Without it the closure is not inlined, so
709+
the mutable slab temporary it writes to escapes and is heap-allocated once per
710+
slab; that measured 1.1-3.8x slower across the operator kernels. (Forcing the
711+
inline from inside these helpers does not work: wrapping `f` in another
712+
closure to annotate its call site reintroduces the allocation.)
713+
- `@inbounds`, since a closure body does not inherit it from an enclosing
714+
`@inbounds` block.
715+
"""
716+
@inline foreach_axis(f::F, op::SpectralElementOperator) where {F} =
717+
unrolled_foreach(f, axis_vals(op))
718+
719+
@inline sum_axes(f::F, op::SpectralElementOperator) where {F} =
720+
unrolled_reduce(+, unrolled_map(f, axis_vals(op)))
721+
694722
"""
695723
slab_dims(op, Nq)
696724
@@ -819,8 +847,8 @@ Base.@propagate_inbounds function apply_operator(
819847
@inbounds for ij in node_indices(space)
820848
local_geometry = get_local_geometry(space, ij, slabidx)
821849
v = get_node(space, arg, ij, slabidx)
822-
unrolled_foreach(axis_vals(op)) do vd
823-
@inline # `out` heap-allocates per slab if this closure is not inlined
850+
foreach_axis(op) do vd
851+
@inline
824852
@inbounds begin
825853
i = ij[axis_index(vd)]
826854
Jvᵈ =
@@ -958,8 +986,8 @@ Base.@propagate_inbounds function apply_operator(
958986
node = slab_node_index(ij)
959987
local_geometry = get_local_geometry(space, ij, slabidx)
960988
u = get_node(space, arg1, ij, slabidx)
961-
unrolled_foreach(axis_vals(op)) do vd
962-
@inline # `Ju` heap-allocates per slab if this closure is not inlined
989+
foreach_axis(op) do vd
990+
@inline
963991
@inbounds Ju[axis_index(vd)][node] =
964992
local_geometry.J * contravariant(vd, u, local_geometry)
965993
end
@@ -970,8 +998,8 @@ Base.@propagate_inbounds function apply_operator(
970998
fill!(parent(out), zero(FT))
971999
@inbounds for ij in node_indices(space)
9721000
node = slab_node_index(ij)
973-
unrolled_foreach(axis_vals(op)) do vd
974-
@inline # `out` heap-allocates per slab if this closure is not inlined
1001+
foreach_axis(op) do vd
1002+
@inline
9751003
@inbounds begin
9761004
i = ij[axis_index(vd)]
9771005
Juᵈ = Ju[axis_index(vd)]
@@ -1046,8 +1074,8 @@ Base.@propagate_inbounds function apply_operator(
10461074
@inbounds for ij in node_indices(space)
10471075
local_geometry = get_local_geometry(space, ij, slabidx)
10481076
x = form_weighted_arg(form, local_geometry, get_node(space, arg, ij, slabidx))
1049-
unrolled_foreach(axis_vals(op)) do vd
1050-
@inline # `out` heap-allocates per slab if this closure is not inlined
1077+
foreach_axis(op) do vd
1078+
@inline
10511079
@inbounds begin
10521080
i = ij[axis_index(vd)]
10531081
for k in 1:Nq
@@ -1195,8 +1223,8 @@ Base.@propagate_inbounds function apply_operator(
11951223
local_geometry = get_local_geometry(space, ij, slabidx)
11961224
v = get_node(space, arg, ij, slabidx)
11971225
u = curl_covariant_components(op, form, v, local_geometry)
1198-
unrolled_foreach(axis_vals(op)) do vd
1199-
@inline # `out` heap-allocates per slab if this closure is not inlined
1226+
foreach_axis(op) do vd
1227+
@inline
12001228
@inbounds begin
12011229
i = ij[axis_index(vd)]
12021230
for k in 1:Nq

0 commit comments

Comments
 (0)