- Notifications
You must be signed in to change notification settings - Fork67
Global scan#665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Global scan#665
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…but not working yet
…but not working yet
[CI]: Can one of the admins verify this patch? |
| #ifndef NBL_BUILTIN_MAX_SCAN_LEVELS | ||
| #define NBL_BUILTIN_MAX_SCAN_LEVELS7 | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
do asNBL_CONSTEXPR instead of define
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
also this better be formulated slightly differently, in terms of reduction passes, not reduction + downsweep
| uint32_t topLevel; | ||
| uint32_t temporaryStorageOffset[NBL_BUILTIN_MAX_SCAN_LEVELS/2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
use uint16_t for this
| uint32_t temporaryStorageOffset[NBL_BUILTIN_MAX_SCAN_LEVELS/2]; | ||
| }; | ||
| Parameters_tgetParameters(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no need for such a forward decl anymore
| struct DefaultSchedulerParameters_t | ||
| { | ||
| uint32_t finishedFlagOffset[NBL_BUILTIN_MAX_SCAN_LEVELS-1]; | ||
| uint32_t cumulativeWorkgroupCount[NBL_BUILTIN_MAX_SCAN_LEVELS]; | ||
| }; | ||
| DefaultSchedulerParameters_tgetSchedulerParameters(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
split out schedulers into separate headers
| template<typename Storage_t> | ||
| voidgetData( | ||
| inoutStorage_t data, | ||
| inuint levelInvocationIndex, | ||
| inuint localWorkgroupIndex, | ||
| inuint treeLevel, | ||
| inuint pseudoLevel | ||
| NBL_REF_ARG(Storage_t) data, | ||
| NBL_CONST_REF_ARG(uint32_t) levelInvocationIndex, | ||
| NBL_CONST_REF_ARG(uint32_t) localWorkgroupIndex, | ||
| NBL_CONST_REF_ARG(uint32_t) treeLevel, | ||
| NBL_CONST_REF_ARG(uint32_t) pseudoLevel | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| #define _NBL_HLSL_SCAN_GET_PADDED_DATA_DECLARED_ | ||
| #endif | ||
| #ifndef _NBL_HLSL_SCAN_SET_DATA_DECLARED_ | ||
| namespace nbl | ||
| { | ||
| namespace hlsl | ||
| { | ||
| namespace scan | ||
| { | ||
| template<typename Storage_t> | ||
| voidsetData( | ||
| inStorage_t data, | ||
| inuint levelInvocationIndex, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no more forward declarations, just let it be an accessor.
Also thelevel const-ref-args can be rolled up into a single structSDataIndex
| // Copyright (C) 2023 - DevSH Graphics Programming Sp. z O.O. | ||
| // This file is part of the "Nabla Engine". | ||
| // For conditions of distribution and use, see copyright notice in nabla.h | ||
| #ifndef _NBL_HLSL_SCAN_DESCRIPTORS_INCLUDED_ | ||
| #define _NBL_HLSL_SCAN_DESCRIPTORS_INCLUDED_ | ||
| // choerent -> globallycoherent | ||
| #include"nbl/builtin/hlsl/scan/declarations.hlsl" | ||
| #include"nbl/builtin/hlsl/workgroup/basic.hlsl" | ||
| // coherent -> globallycoherent | ||
| namespace nbl | ||
| { | ||
| namespace hlsl | ||
| { | ||
| namespace scan | ||
| { | ||
| template<uint32_t scratchElementCount=scratchSz>// (REVIEW): This should be externally defined. Maybe change the scratch buffer to RWByteAddressBuffer? Annoying to manage though... | ||
| struct Scratch | ||
| { | ||
| uint32_t workgroupsStarted; | ||
| uint32_t data[scratchElementCount]; | ||
| }; | ||
| [[vk::binding(0 ,0)]]RWStructuredBuffer<uint32_t/*Storage_t*/> scanBuffer;// (REVIEW): Make the type externalizable. Decide how (#define?) | ||
| [[vk::binding(1 ,0)]]RWStructuredBuffer<Scratch>globallycoherent scanScratchBuf;// (REVIEW): Check if globallycoherent can be used with Vulkan Mem Model | ||
| template<typename Storage_t,bool isExclusive=false> | ||
| voidgetData( | ||
| NBL_REF_ARG(Storage_t) data, | ||
| NBL_CONST_REF_ARG(uint32_t) levelInvocationIndex, | ||
| NBL_CONST_REF_ARG(uint32_t) localWorkgroupIndex, | ||
| NBL_CONST_REF_ARG(uint32_t) treeLevel, | ||
| NBL_CONST_REF_ARG(uint32_t) pseudoLevel | ||
| ) | ||
| { | ||
| const Parameters_t params =getParameters();// defined differently for direct and indirect shaders | ||
| uint32_t offset = levelInvocationIndex; | ||
| constbool notFirstOrLastLevel =bool(pseudoLevel); | ||
| if (notFirstOrLastLevel) | ||
| offset += params.temporaryStorageOffset[pseudoLevel-1u]; | ||
devshgraphicsprogrammingApr 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
remove this whole file, it should be userspace
| // TODO: Can we make it a static variable? | ||
| groupshared uint32_t wgScratch[SharedScratchSz]; | ||
| #include"nbl/builtin/hlsl/workgroup/arithmetic.hlsl" | ||
| template<uint16_t offset> | ||
| struct WGScratchProxy | ||
| { | ||
| uint32_tget(const uint32_t ix) | ||
| { | ||
| return wgScratch[ix+offset]; | ||
| } | ||
| voidset(const uint32_t ix,const uint32_t value) | ||
| { | ||
| wgScratch[ix+offset] = value; | ||
| } | ||
| uint32_tatomicAdd(uint32_t ix, uint32_t val) | ||
| { | ||
| return glsl::atomicAdd(wgScratch[ix + offset], val); | ||
| } | ||
| voidworkgroupExecutionAndMemoryBarrier() | ||
| { | ||
| nbl::hlsl::glsl::barrier(); | ||
| //nbl::hlsl::glsl::memoryBarrierShared(); implied by the above | ||
| } | ||
| }; | ||
| static WGScratchProxy<0> accessor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
scratches are userspace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
use accessors
| /** | ||
| * Required since we rely on SubgroupContiguousIndex instead of | ||
| * gl_LocalInvocationIndex which means to match the global index | ||
| * we can't use the gl_GlobalInvocationID but an index based on | ||
| * SubgroupContiguousIndex. | ||
| */ | ||
| uint32_tglobalIndex() | ||
| { | ||
| return nbl::hlsl::glsl::gl_WorkGroupID().x*WORKGROUP_SIZE+nbl::hlsl::workgroup::SubgroupContiguousIndex(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we have this in a header already
| struct ScanPushConstants | ||
| { | ||
| nbl::hlsl::scan::Parameters_t scanParams; | ||
| nbl::hlsl::scan::DefaultSchedulerParameters_t schedulerParams; | ||
| }; | ||
| [[vk::push_constant]] | ||
| ScanPushConstants spc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
everything affecting the pipeline layout should be userspace
| #ifndef _NBL_HLSL_MAIN_DEFINED_ | ||
| [numthreads(_NBL_HLSL_WORKGROUP_SIZE_,1,1)] | ||
| voidCSMain() | ||
| [numthreads(WORKGROUP_SIZE,1,1)] | ||
| voidmain() | ||
| { | ||
| if (bool(nbl::hlsl::scan::getIndirectElementCount())) | ||
| nbl::hlsl::scan::main(); | ||
| if(bool(nbl::hlsl::scan::getIndirectElementCount())) { | ||
| // TODO call main from virtual_workgroup.hlsl | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
all this should be userspace, also there will be very little difference between direct and indirect
| { | ||
| template<class Binop,class Storage_t> | ||
| voidvirtualWorkgroup(inuinttreeLevel,inuintlocalWorkgroupIndex) | ||
| template<class Binop,typename Storage_t,bool isExclusive, uint16_t ItemCount,class Accessor,class device_capabilities=void> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why not template on the workgroup scan instead?
Within you can alias and extract:
- binop
- storage_t
- exclusive or not
- item count
- smem accessor
- device traits necessary
| const uint32_t levelInvocationIndex = localWorkgroupIndex * glsl::gl_WorkGroupSize().x +SubgroupContiguousIndex(); | ||
| constbool lastInvocationInGroup =SubgroupContiguousIndex() == (gl_WorkGroupSize().x -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn'tItemCount be used instead ofglsl::gl_WorkGroupSize().x ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
most definitely
| // could do scanScratchBuf[0u].workgroupsStarted[SubgroupContiguousIndex()] = 0u but don't know how many invocations are live during this call | ||
| if(workgroup::SubgroupContiguousIndex() == 0u) | ||
| { | ||
| for(uint32_t i =0; i < params.topLevel; i++) | ||
| { | ||
| scanScratchBuf[0u].workgroupsStarted[i] = 0u; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
don't you know the workgroup size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My bad, this is actually not used anymore. I think it was being used inside aninRage at some point? Not sure. However we can't reset theworkgroupStarted buffer within the shader at the end of the Reduce step (which was the initial goal) because it's possible that there are still workgroups that are "alive" where they won't be doing any work but will still increase theworkgroupStarted buffer and some times we end up doing the resetbefore those WGs exit, ending up with some 1 values instead of all 0s.
…t symbols for CReduce and CScanner
Description
Initial implementation of the Global Scan in HLSL
Testing
Not yet finished
TODO list:
Still need to test the HLSL code as well as the porting of the example code to the new API of vulkan_1_3 branch