Device-wide chained scan with lookback by keptsecret · Pull Request #1086 · Devsh-Graphics-Programming/Nabla

keptsecret · 2026-07-30T09:47:09Z

No description provided.

…probably not needed but left in for now

Erfan-Ahmadi · 2026-08-05T07:55:08Z

+        return spirv::atomicSMin<T>(ptr, spv::ScopeDevice, spv::MemorySemanticsMaskNone, value);
+    }
+
+    template<typename Ptr_T> // DXC Workaround


It's best to say exactly what dxc issue we're working around and be more detailed in the comments. in case they fix it and we can upgrade our code.
I think it's something to do with passing groupshared or other address qualifiers here.

I have a question though.
These don't need to be cpp compatible, it's gpu specific, why are we using NBL_REF_ARG and both REQ_TOP and REQ_BOT (hlsl enable_if and c++20 requires). I think BOT is enough?

Side note:
Just putting it out there that I don't like TOP/BOT naming 😆 I'd prefer something along the lines of:

NBL_HOST_CONCEPT + NBL_DEVICE_CONCEPT or NBL_DEVICE_SFINAE. which is more clear

These don't need to be cpp compatible, it's gpu specific, why are we using NBL_REF_ARG and both REQ_TOP and REQ_BOT (hlsl enable_if and c++20 requires). I think BOT is enough?

Because in C++20 you need the requires clause on the top, but enable_if workaround needs it in the bottom.

Thats the convention we already chose and if we were to change it we'd have to change a lot of other code as well.

I have a question though.
These don't need to be cpp compatible, it's gpu specific, why are we using NBL_REF_ARG and both REQ_TOP and REQ_BOT (hlsl enable_if and c++20 requires). I think BOT is enough?

you want to be forward compatible with C++20, because HLSL is going that way, also it keeps the code easy to port to CUDA and SYCL later on.

It's best to say exactly what dxc issue we're working around and be more detailed in the comments. in case they fix it and we can upgrade our code.
I think it's something to do with passing groupshared or other address qualifiers here.

Yes @keptsecret please cite/link the DXC issue in the comments for stuff like that (git commit message too)

Erfan-Ahmadi · 2026-08-10T04:06:19Z

+    void __call(NBL_REF_ARG(DataAccessor) dataAccessor, NBL_REF_ARG(ScratchAccessor) scratchAccessor, NBL_REF_ARG(ReductionAccessor) workgroupReduction, NBL_REF_ARG(WorkgroupCounter) workgroupCounter)
+    {        
+        const uint16_t invocIx = workgroup::SubgroupContiguousIndex();
+        if (!invocIx)


For readability purposes if you want to check if something is equal to zero, it's best to just do ==0 instead of treating as a boolean

Erfan-Ahmadi · 2026-08-10T05:12:44Z

+
+        uint16_t workgroupId;
+        scratchAccessor.template get<uint32_t, uint32_t>(0u, workgroupId);
+        scratchAccessor.workgroupExecutionAndMemoryBarrier();


you don't need a workgroupExecutionAndMemoryBarrier here.

Erfan-Ahmadi · 2026-08-10T06:02:19Z

+                workgroup2::exclusive_scan<Config,BinOp,device_capabilities>::template __call<wg_data_proxy_t, ScratchAccessor>(wgDataAccessor, scratchAccessor);
+            else
+                workgroup2::inclusive_scan<Config,BinOp,device_capabilities>::template __call<wg_data_proxy_t, ScratchAccessor>(wgDataAccessor, scratchAccessor);
+            scratchAccessor.workgroupExecutionAndMemoryBarrier();


I don't think you need a workgroupExecutionAndMemoryBarrier Because you want to access the preloaded array which is thread local.

Erfan-Ahmadi · 2026-08-10T06:04:55Z

+            currGroupReduction = wgDataAccessor.preloaded[wg_data_proxy_t::PreloadedDataCount-1u][Config::ItemsPerInvocation_0-1u];
+            if (Exclusive)
+                currGroupReduction = binop(currGroupReduction, lastElem);
+            if (invocIx == lastInvocIx)
+                scratchAccessor.template set<scalar_t, uint32_t>(0u, currGroupReduction);


You only need to do this for the last invocation.
So maybe you can encompass all of it under if (invocIx == lastInvocIx)

Erfan-Ahmadi · 2026-08-10T06:33:57Z

+        if (workgroupId)
+        {
+            bool locked = sIsLocked;
+            scratchAccessor.workgroupExecutionAndMemoryBarrier();


why do workgroups need to sync here?

Erfan-Ahmadi · 2026-08-10T06:56:02Z

+                scratchAccessor.workgroupExecutionAndMemoryBarrier();
+
+                locked = sIsLocked;
+                scratchAccessor.workgroupExecutionAndMemoryBarrier();


why workgroupExecutionAndMemoryBarrier here ?

Erfan-Ahmadi · 2026-08-10T07:04:56Z

+
+                locked = sIsLocked;
+                scratchAccessor.workgroupExecutionAndMemoryBarrier();
+                if (locked)


Add comment:
Fall back path: we spun to MaxSpinCount But no previous workgroup had there global reduction ready (Flag_Inclusive).
So we try to do reduction for all previous work groups one by one until we reach Flag_Inclusive

Erfan-Ahmadi · 2026-08-10T07:22:58Z

+                        const scalar_t storeVal = hlsl::mix(Flag_Inclusive, Flag_Reduction, fallbackGroupId > 0u) | (fallbackReduction << Flag_Shift);
+                        const scalar_t fallbackPayload = workgroupReduction.atomicMax(fallbackGroupId, storeVal);
+
+                        prevReduction = binop(prevReduction, hlsl::mix(fallbackReduction, fallbackPayload >> Flag_Shift, fallbackPayload > scalar_t(0.0)));


I get why you're doing atomic Max here, You want to take the inclusive one if the original workgroup Finished after we calculated the reduction redundantly
But I don't get the mix here. Why not take the fall back payload all the time?

Erfan-Ahmadi · 2026-08-10T07:25:47Z

+                        if (fallbackGroupId == 0u || (fallbackPayload & Flag_Mask) == Flag_Inclusive)
+                        {
+                            const scalar_t storeVal = Flag_Inclusive | (binop(prevReduction, currGroupReduction) << Flag_Shift);
+                            workgroupReduction.atomicExchange(workgroupId, storeVal);


Be careful again, you need some sort of memory semantics to ensure correct memory barriers when different work groups access the same value atomically.

Erfan-Ahmadi · 2026-08-10T07:34:27Z

+                    scratchAccessor.workgroupExecutionAndMemoryBarrier();
+
+                    locked = sIsLocked;
+                    scratchAccessor.workgroupExecutionAndMemoryBarrier();


why workgroupExecutionAndMemoryBarrier here ?

Erfan-Ahmadi · 2026-08-10T07:35:01Z

+                    wg_data_proxy_t fallbackDataAccessor = wg_data_proxy_t::create(dataAccessor.getInputBufAddr(), dataAccessor.getOutputBufAddr(), fallbackGroupId);
+                    fallbackDataAccessor.preload();
+                    scalar_t fallbackReduction = workgroup2::reduction<Config,BinOp,device_capabilities>::template __call<wg_data_proxy_t, ScratchAccessor>(fallbackDataAccessor, scratchAccessor);
+                    scratchAccessor.workgroupExecutionAndMemoryBarrier();


why workgroupExecutionAndMemoryBarrier here ?

devshgraphicsprogramming · 2026-08-24T11:03:23Z

+template<typename T, typename V, typename I=uint32_t>
+NBL_BOOL_CONCEPT ArithmeticSharedMemoryAccessor = concepts::accessors::GenericSharedMemoryAccessor<T,V,I>;
+
+template<typename T, typename V, typename I=uint32_t>
+NBL_BOOL_CONCEPT ArithmeticReadOnlyDataAccessor = concepts::accessors::GenericReadAccessor<T,V,I>;
+
+template<typename T, typename V, typename I=uint32_t>
+NBL_BOOL_CONCEPT ArithmeticDataAccessor = concepts::accessors::GenericDataAccessor<T,V,I>;


why are we introducing new aliases?

devshgraphicsprogramming · 2026-08-24T11:08:37Z

+#define NBL_CONCEPT_NAME DeviceReductionsAccessor
+#define NBL_CONCEPT_TPLT_PRM_KINDS (typename)(typename)
+#define NBL_CONCEPT_TPLT_PRM_NAMES (T)(V)
+#define NBL_CONCEPT_PARAM_0 (accessor, T)
+#define NBL_CONCEPT_PARAM_1 (val, V)
+#define NBL_CONCEPT_PARAM_2 (index, uint64_t)
+NBL_CONCEPT_BEGIN(3)
+#define accessor NBL_CONCEPT_PARAM_T NBL_CONCEPT_PARAM_0
+#define val NBL_CONCEPT_PARAM_T NBL_CONCEPT_PARAM_1
+#define index NBL_CONCEPT_PARAM_T NBL_CONCEPT_PARAM_2
+NBL_CONCEPT_END(
+    ((NBL_CONCEPT_REQ_TYPE_ALIAS_CONCEPT)(concepts::accessors::GenericDataAccessor, T, V, uint64_t))
+    ((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((accessor.atomicMax(index, val)), is_same_v, V))
+    ((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((accessor.atomicExchange(index, val)), is_same_v, V))
+);
+#undef val
+#undef index
+#undef accessor
+#include <nbl/builtin/hlsl/concepts/__end.hlsl>
+


shall we generalize more and hoist it out into a GenericAtomicAccessor (no arithmetic)

devshgraphicsprogramming · 2026-08-24T11:08:45Z

+#undef accessor
+#include <nbl/builtin/hlsl/concepts/__end.hlsl>
+
+// TODO: as counter, maybe just increment 1 always?


nah don't bother

devshgraphicsprogramming · 2026-08-24T11:09:36Z

+#define NBL_CONCEPT_NAME WorkgroupCounterAccessor
+#define NBL_CONCEPT_TPLT_PRM_KINDS (typename)
+#define NBL_CONCEPT_TPLT_PRM_NAMES (T)
+#define NBL_CONCEPT_PARAM_0 (accessor, T)
+#define NBL_CONCEPT_PARAM_1 (val, uint32_t)
+#define NBL_CONCEPT_PARAM_2 (index, uint64_t)
+NBL_CONCEPT_BEGIN(3)
+#define accessor NBL_CONCEPT_PARAM_T NBL_CONCEPT_PARAM_0
+#define val NBL_CONCEPT_PARAM_T NBL_CONCEPT_PARAM_1
+#define index NBL_CONCEPT_PARAM_T NBL_CONCEPT_PARAM_2
+NBL_CONCEPT_END(
+    ((NBL_CONCEPT_REQ_EXPR_RET_TYPE)((accessor.atomicAdd(index, val)), is_same_v, uint32_t))
+);
+#undef val
+#undef index
+#undef accessor
+#include <nbl/builtin/hlsl/concepts/__end.hlsl>


same could generalize to GenericAtomicIntAccessor or GenericAtomicArithmeticAccessor

devshgraphicsprogramming · 2026-08-24T11:14:19Z

+namespace impl
+{
+template<typename T NBL_STRUCT_CONSTRAINABLE>
+struct atomicMin;
+
+template<typename T>
+NBL_PARTIAL_REQ_TOP(concepts::SignedIntegral<T>)
+struct atomicMin<T NBL_PARTIAL_REQ_BOT(concepts::SignedIntegral<T>) >
+{
+    static T __call(NBL_REF_ARG(T) ptr, T value)
+    {
+        return spirv::atomicSMin<T>(ptr, spv::ScopeDevice, spv::MemorySemanticsMaskNone, value);
+    }
+
+    template<typename Ptr_T> // DXC Workaround
+    static T __call(Ptr_T ptr, T value)
+    {
+        return spirv::atomicSMin<T>(ptr, spv::ScopeDevice, spv::MemorySemanticsMaskNone, value);
+    }
+};
+
+template<typename T>
+NBL_PARTIAL_REQ_TOP(concepts::UnsignedIntegral<T>)
+struct atomicMin<T NBL_PARTIAL_REQ_BOT(concepts::UnsignedIntegral<T>) >
+{
+    static T __call(NBL_REF_ARG(T) ptr, T value)
+    {
+        return spirv::atomicUMin<T>(ptr, spv::ScopeDevice, spv::MemorySemanticsMaskNone, value);
+    }
+
+    template<typename Ptr_T> // DXC Workaround
+    static T __call(Ptr_T ptr, T value)
+    {
+        return spirv::atomicUMin<T>(ptr, spv::ScopeDevice, spv::MemorySemanticsMaskNone, value);
+    }
+};
+
+template<typename T NBL_STRUCT_CONSTRAINABLE>
+struct atomicMax;
+
+template<typename T>
+NBL_PARTIAL_REQ_TOP(concepts::SignedIntegral<T>)
+struct atomicMax<T NBL_PARTIAL_REQ_BOT(concepts::SignedIntegral<T>) >
+{
+    static T __call(NBL_REF_ARG(T) ptr, T value)
+    {
+        return spirv::atomicSMax<T>(ptr, spv::ScopeDevice, spv::MemorySemanticsMaskNone, value);
+    }
+
+    template<typename Ptr_T> // DXC Workaround
+    static T __call(Ptr_T ptr, T value)
+    {
+        return spirv::atomicSMax<T>(ptr, spv::ScopeDevice, spv::MemorySemanticsMaskNone, value);
+    }
+};
+
+template<typename T>
+NBL_PARTIAL_REQ_TOP(concepts::UnsignedIntegral<T>)
+struct atomicMax<T NBL_PARTIAL_REQ_BOT(concepts::UnsignedIntegral<T>) >
+{
+    static T __call(NBL_REF_ARG(T) ptr, T value)
+    {
+        return spirv::atomicUMax<T>(ptr, spv::ScopeDevice, spv::MemorySemanticsMaskNone, value);
+    }
+
+    template<typename Ptr_T> // DXC Workaround
+    static T __call(Ptr_T ptr, T value)
+    {
+        return spirv::atomicUMax<T>(ptr, spv::ScopeDevice, spv::MemorySemanticsMaskNone, value);
+    }
+};
+}


we actually have a huge problem here and I think this is why I didn't pour that much effort into the GLSL compat functions

You don't know the address space so don't know the scope to emit by default - comment at the top of the atomic code block

memory semantics are always None

Ideally none of our code should be using the GLSL atomics because they're so bad and imprecise

keptsecret added 22 commits July 14, 2026 16:57

chained scan initial commit w rough outline

007f23e

implement proper read/write with separate data accessor for workgroup

9604ca6

use correct binop

4192826

minor fix to creating wg accessor

b132a2e

use a separate accessor type for reduction buf

b46cad2

fix some compilation bugs, still not running correct

59219c5

fix some bugs in chained scan, still need to add lookback fallback

a056aeb

lookback fallback to avoid spin

12b2d9b

implement atomic min and max

068776e

some bug fixes to atomic min/max

747d35b

a number of bug fixes to indexing stuff

972a8fb

fix exclusive scan to reduction store

9085b5d

use a workgroup counter to determine execution order

3a59e93

fix for indexing in workgroup scan accessor (unsure if temp fix)

25e2d01

fix lookback to spin a bit for some more consistency

ba9e151

can use single scratch address + fixes bug in example, memorybarrier …

dd61813

…probably not needed but left in for now

fixes to indexing in preload

02d6966

Merge branch 'master' into device_chained_scan

833081c

add chain scan hlsl to cmakelist

e5153a9

latest example

fbf4b57

concepts for accessors of device scan, removed obsolete comments

c5b3031

latest example

12a8f56

Erfan-Ahmadi reviewed Aug 10, 2026

View reviewed changes

devshgraphicsprogramming reviewed Aug 24, 2026

View reviewed changes

Uh oh!

Conversation

keptsecret commented Jul 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devshgraphicsprogramming Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

devshgraphicsprogramming Aug 24, 2026 •

edited

Loading