Unify DataLayouts and simplify API - #2522
Conversation
8d0a64b to
c5b5c9c
Compare
c5b5c9c to
6137f53
Compare
ba2cc32 to
6c38000
Compare
c3a7fec to
6a6d558
Compare
The CPU hierarchy is fixed, and I doubt it will ever need to get more complicated. I was initially hoping to keep the GPU hierarchy simple as well, but I had to add a bit more complexity to match ClimaCore's current behavior. Aside from some naming simplifications ( When processing slice views in parallel across
This will match the current implementation of
To minimize the amount of code we'll need in the CUDA extension, I've handled all of these cases in a single
If the default Let me know if any of this can be changed to further simplify things for you, @Mikolaj-A-Kowalski. |
tapios
left a comment
There was a problem hiding this comment.
This looks good as far as I can tell. It's nice to see the simplifications realized here.
I will send a list of detailed code comments from an audit separately, which identified two apparent reduction bugs.
|
One more thing: Please add a smoke test for the DG Bickley jet (just run it a few steps). I checked it works with this branch, but numerical_flux.jl is not covered by tests; we want to make sure the DG part remains maintained (and soon build out that option). |
imreddyTeja
left a comment
There was a problem hiding this comment.
Have you looked at gpu benchmarks compared to main? It looks like the end to end test is a bit slower
| @inline x_component((; x, y, z)) = | ||
| isone(y) && isone(z) ? x : | ||
| throw(ArgumentError("y and z dimensions in launch configuration are not supported")) |
There was a problem hiding this comment.
Why are the y and z dimensions in the launch configuration not supported?
There was a problem hiding this comment.
This is a limitation of the current design, since nonlinear grid/block dimensions make it much harder to implement automatic coarsening (looping over multiple indices per thread) and automatic launch config adjustment (using the largest possible blocks while making sure they can all run at the same time). Support for y and z dimensions can be added in a future PR, but only if the measurable performance benefits outweigh the increase in complexity.
| } = true | ||
| isascalar(bc) = false | ||
| const MaybeLazyDataLayout = Union{DataLayout, LazyDataLayout} | ||
| const MaybeFusedDataLayoutBroadcast = Union{LazyDataLayout, FusedMultiBroadcast} |
There was a problem hiding this comment.
I think this might be a better name?
| const MaybeFusedDataLayoutBroadcast = Union{LazyDataLayout, FusedMultiBroadcast} | |
| const LazyOrFusedDataLayoutBroadcast = Union{LazyDataLayout, FusedMultiBroadcast} |
There was a problem hiding this comment.
I actually used that name in my first draft of this PR. I ended up replacing it with the current name because LazyDataLayout and FusedMultiBroadcast are both "lazy" objects, so calling the union "lazy or fused" is a bit confusing.
|
I checked the performance of this PR at commit bd53967 vs main. Here are my findings: From profiling prognostic edmf with 1M microphysics in the coupler:
When looking at simple point wise expressions in isolation, I found:
The slowdown is not ideal, but I do not think it is enough of a degradation to prevent merging this. The "tail effect" issue should be an easy fix, and any potential differences in indexing computation should be less impactful for non-trivial point-wise kernels. At the moment, over half the point wise kernels in ClimaAtmos take less than 25 microseconds. That is short enough to where a few integer divisions will be noticeable. Hopefully, fusing more point wise operations will make this a non-problem. |
…erf] Verifies and refines the DataLayouts rewrite across CPUs, GPUs, docs, tests, and downstream packages. - Point indexing: getindex/setindex!/view constant-fold the Cartesian-to-linear conversion and use a constant-stride linear index into the parent of a property-view SubArray, bypassing Base's per-access column-major arithmetic and its linear-to-Cartesian reindex (div/rem, SignedMultiplicativeInverse). This bounds the deeply-inlined finite-difference stencil body that compile-killed the ClimaAtmos EDMF Larcform1 GPU job (OOM): implicit_tendency! compile ~110s -> ~32s, with column FD-op runtime gains and bit-identical values. The offset folds on device via unrolled_reduce (no InvalidIRError). - Property views / IndexStyle: build slice and property SubArrays from Colons (Base.Slice) so VIJHF views are fast-linear (IndexLinear) while VIJFH views stay IndexCartesian; IndexStyle defers to the parent, so pointwise broadcasts never linear-index an IndexCartesian SubArray. - GPU support: DataScopes map onto the CUDA execution hierarchy, loops and reductions launch through auto_launch! with occupancy-based configurations, and Utilities.stable_view keeps slice/property views inference-stable (kernel arguments and closures follow isbits rules). Each view is adapted to a compact device view (Int32 offsets instead of a SubArray) so large EDMF broadcasts fit the sm_60 4 KiB kernel-parameter limit, and DataScopes combine by pairwise recursion so inference does not widen scopes to Any. - CPU runtime matches main: unmasked point loops vectorize under @simd with an inlined point function, nested loops avoid closure allocations, and GPU point loops iterate each thread's strided CartesianIndices subset through an indexable isbits wrapper, keeping kernel launch latency at main's level. - Compile time: @maybe_propagate_inbounds requests inlining of stencil expression nodes only when check-bounds is off, taking the FCT advection examples from 40-58 minutes to about a minute in CI. - Reductions and masks: order-insensitive pairwise safe_mapreduce (no linear-indexing assumption), masks are keyword arguments, equality ignores padding, and field2array / the distributed HDF5 writer handle all layout shapes. Fix the GPU MPI DSS exchange buffer index that overlapped items when Nv > 1 and Nf > 1, and apply the QuasiMonotoneLimiter through one scalar view per component (avoiding an uncompilable reshape of a device SubArray). - Compatibility, docs, tests: deprecation aliases (DataLayouts/deprecated.jl, Fields.ColumnField) and the universal CartesianIndex{5} convention keep downstream packages working; zero-size fields stay hidden from propertynames; docs and NEWS are expanded and Aqua ambiguities resolved; tests restore check_basetype, benchmark_fill, and the VIJFH-F64 stencil set, and add DataLayouts mask/reduction jobs, device-aware dss tests, and latency baselines at their original tolerance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Purpose
This PR completes the first task outlined in #2468, unifying most of the structs in the
DataLayoutsmodule and simplifying their API to eliminate code duplication. This is a prerequisite for #2514.All loops and reductions over layouts now run through two scope-dispatched primitives,
foreach_sliceandreduce_points, which work identically on CPUs (including nested multithreading) and GPUs (as single kernels). The rewrite is verified to compile for GPUs without runtime dispatch, and hot paths match the performance ofmainon CPUs.Tested against the ClimaAtmos CI pipeline in this build (only a single MSE test failure due to roundoff error).
Content
AbstractDatatoDataLayout, and make it a subtype ofAbstractArrayDataF,VIJHWithF,VIH1, andIH1JH2MArrays or cuda shmem arrays) bysimilar(::DataLayout, ...)Faxisgetindexandsetindex!match standardAbstractArraybehaviorIndexStyle, instead of separate structs likeNonExtrudedBroadcastedDataScopesingleton, a more versatile generalization of aClimaComms.AbstractDeviceDataLayouts, including broadcasts and reductionsDataScopes, as well as rules for partitioning into smallerDataScopesDataScopepartitionsDataScopedispatch:foreach_sliceandreduce_pointsfill!,copyto!, andfused_copyto!through theforeach_pointprimitive (a simple wrapper forforeach_slice)reduceandfused_copyto!fused_copyto!treat all layouts consistently (and actually fuse thecopyto!loops every time it is called)BroadcastStyles to one concrete struct, still calledDataStyleDataStyleto propagate the layout type, ignoringDataFlayouts when combining distinct typesshape_paramsAbstractArrays inBaseRefs, and for simple identity function broadcastsTuples in broadcasts are treated asRefsTuples fall back to the default behavior forAbstractArrays instead of erroringUtilities.stable_view, which constructs inference-stableSubArrays for all slice and property views (GPUArrays replaces contiguousCuArrayviews with uninferrable derived arrays), eliminating reshaped views on GPUs altogetherDataLayouts/deprecated.jlwith exported aliases forAbstractData,IJFH, andIJHF(downstream packages and registered satellite packages still reference them)Roadmap
A suggested order for reviewing the changes:
src/DataLayouts/DataLayouts.jl,src/DataLayouts/struct_storage.jl,src/DataLayouts/scopes.jl,src/DataLayouts/deprecated.jlsrc/DataLayouts/indexing.jl,src/DataLayouts/broadcast.jl,src/DataLayouts/masks.jl,src/DataLayouts/loops.jl,src/Utilities/safe_mapreduce.jl,src/Utilities/Utilities.jlext/cuda/scopes.jl,ext/cuda/loops.jl,ext/cuda/cuda_utils.jl,ext/cuda/data_layouts.jl,ext/cuda/adapt.jl(v, i, j, h)indices, 5-D parent arrays,shape_params):src/Fields/,src/Spaces/,src/Grids/,src/Operators/,src/MatrixFields/,src/Limiters/,src/Topologies/dss*.jl,src/Remapping/,src/InputOutput/, remainingext/cuda/files,lib/ClimaCoreMakie,lib/ClimaCorePlots,examples/test/DataLayouts/unit_loops.jl,test/Utilities/unit_stable_view.jl, updatedtest/DataLayouts/,test/Fields/,test/Spaces/,test/Operators/, and GPU expectation updates (test/gpu/latency_benchmarks.jl,test/Spaces/opt_spaces.jl,test/Operators/finitedifference/opt_examples.jl)