Refactor ParticleAttrib::scatter() - #585
Open
aaadelmann wants to merge 4 commits into
Open
Conversation
…in scatter” decision is made on the host before launching the Kokkos kernel.
**Purpose**
The old code did this inside every particle iteration:
```cpp
size_t mapped_idx = useHashView ? hash_array(idx) : idx;
```
`useHashView` is a runtime boolean captured into the kernel. That means every particle pays for a branch, and the kernel body always mentions `hash_array(idx)` even when no hash view is used.
The new code splits this into two template instantiations:
```cpp
scatterImpl<true>(...)
scatterImpl<false>(...)
```
Inside the kernel it becomes:
```cpp
size_t mapped_idx = idx;
if constexpr (UseHashView) {
mapped_idx = hash_array(idx);
}
```
So for the non-hashed path, the hash-array access is compiled out entirely.
**Secondary Purpose**
The change also moves the hash extent check out of the kernel implementation wrapper and normalizes the type:
```cpp
const auto hashExtent = static_cast<decltype(iteration_policy.end())>(hash_array.extent(0));
```
This avoids the Kokkos 5.2 OpenMP/GCC signed/unsigned warning from comparing `iteration_policy.end()` with `hash_array.extent(0)` directly.
**Behavioral Impact**
The public scatter API does not change.
Expected behavior stays the same:
- no hash array: `mapped_idx = idx`
- hash array present: `mapped_idx = hash_array(idx)`
- hash array too small for the requested iteration policy: abort with the existing diagnostic
This is mainly a compile-time dispatch/performance/cleanup change:
- removes one per-particle runtime branch
- avoids compiling hash access into the non-hash kernel
- fixes the signed/unsigned comparison warning
- keeps the existing bounds check and scatter semantics
** ToDo
- Test on GPUs, on CPU (MAC) no improvement
- The current cast is probably fine for realistic particle counts, but a more defensive version could use a named policy index type and possibly check representability before casting. For IPPL’s current usage, this is not a practicaxl concern.
Port the CUDA-safe hashed scatter implementation from fixissue/#415 onto the hashed-scatter branch. Move the kernel-bearing scatter implementation out of the private ParticleAttrib member scope and into a namespace-scope detail helper. CUDA extended host-device lambdas cannot be enclosed by private or protected class member functions, so keeping the KOKKOS_LAMBDA inside the former private scatterImpl caused GH200/NVCC builds to fail. Keep ParticleAttrib::scatter as the public host-side dispatcher. It validates the optional hash view and selects the hashed or non-hashed implementation via: particleAttribScatterImpl<true> particleAttribScatterImpl<false> Route mapped-index selection through a small KOKKOS_INLINE_FUNCTION helper so the hash lookup is compiled only for the hashed instantiation and is not first captured inside an if constexpr context. Also allow CIC scatter to accept a value type that differs from the field value type. This enables mixed-value scatter such as: ParticleAttrib<float> -> Field<double> by casting the scattered value once to the field view value type before accumulating into the grid. Add regression coverage for: - plain ParticleAttrib<float> -> Field<double> scatter - hashed ParticleAttrib<float> -> Field<double> scatter - dimensions 1, 2, and 3 through GatherScatterTest Tested locally with: cmake --build build-fixissue-415-merge --target GatherScatterTest -j 8 ctest --test-dir build-fixissue-415-merge --output-on-failure -R '^GatherScatterTest$' mpirun -np 2 build-fixissue-415-merge/unit_tests/Particle/GatherScatterTest --gtest_filter='*MixedValueType*' mpirun -np 4 build-fixissue-415-merge/unit_tests/Particle/GatherScatterTest --gtest_filter='*MixedValueType*' All tests passed.
aaadelmann
marked this pull request as ready for review
August 6, 2026 19:13
Collaborator
|
That's actually a nice solution: you now use a template to decide which scatter to use, allowing the compiler to optimize the ternary operation. However, "on CPU (MAC) no improvement" is to be expected: one conditional statement is (in my opinion) negligible compared to the scatter kernel itself. It's even possible that right now it uses branch predictions, which would leading to no performance difference at all. When I implemented it, I didn't want code duplication or change templates inside IPPL and settled for a "non-divergent" ternary. But this is for sure the best solution. If you want, I can test this branch against OPALX. |
Member
Author
|
I am now evaluating the performance on LUMI, so no need to OPALX at the moment, thanks Alex ! |
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.
Refactor ParticleAttrib::scatter() so the “hashed scatter” vs “plain scatter” decision is made on the host before launching the Kokkos kernel.
Purpose
The old code did this inside every particle iteration:
size_t mapped_idx = useHashView ? hash_array(idx) : idx;useHashViewis a runtime boolean captured into the kernel. That means every particle pays for a branch, and the kernel body always mentionshash_array(idx)even when no hash view is used.The new code splits this into two template instantiations:
Inside the kernel it becomes:
So for the non-hashed path, the hash-array access is compiled out entirely.
Secondary Purpose
The change also moves the hash extent check out of the kernel implementation wrapper and normalizes the type:
This avoids the Kokkos 5.2 OpenMP/GCC signed/unsigned warning from comparing
iteration_policy.end()withhash_array.extent(0)directly.Behavioral Impact
The public scatter API does not change.
Expected behavior stays the same:
mapped_idx = idxmapped_idx = hash_array(idx)This is mainly a compile-time dispatch/performance/cleanup change:
** ToDo
Test on GPUs, on CPU (MAC) no improvement
The current cast is probably fine for realistic particle counts, but a more defensive version could use a named policy index type and possibly check representability before casting. For IPPL’s current usage, this is not a practicaxl concern.