Device-wide chained scan with lookback - #1086
Conversation
…probably not needed but left in for now
| return spirv::atomicSMin<T>(ptr, spv::ScopeDevice, spv::MemorySemanticsMaskNone, value); | ||
| } | ||
|
|
||
| template<typename Ptr_T> // DXC Workaround |
There was a problem hiding this comment.
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_CONCEPTorNBL_DEVICE_SFINAE. which is more clear
| 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) |
There was a problem hiding this comment.
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
|
|
||
| uint16_t workgroupId; | ||
| scratchAccessor.template get<uint32_t, uint32_t>(0u, workgroupId); | ||
| scratchAccessor.workgroupExecutionAndMemoryBarrier(); |
There was a problem hiding this comment.
you don't need a workgroupExecutionAndMemoryBarrier here.
| 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(); |
There was a problem hiding this comment.
I don't think you need a workgroupExecutionAndMemoryBarrier Because you want to access the preloaded array which is thread local.
| 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); |
There was a problem hiding this comment.
You only need to do this for the last invocation.
So maybe you can encompass all of it under if (invocIx == lastInvocIx)
| if (workgroupId) | ||
| { | ||
| bool locked = sIsLocked; | ||
| scratchAccessor.workgroupExecutionAndMemoryBarrier(); |
There was a problem hiding this comment.
why do workgroups need to sync here?
| scratchAccessor.workgroupExecutionAndMemoryBarrier(); | ||
|
|
||
| locked = sIsLocked; | ||
| scratchAccessor.workgroupExecutionAndMemoryBarrier(); |
There was a problem hiding this comment.
why workgroupExecutionAndMemoryBarrier here ?
|
|
||
| locked = sIsLocked; | ||
| scratchAccessor.workgroupExecutionAndMemoryBarrier(); | ||
| if (locked) |
There was a problem hiding this comment.
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
| 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))); |
There was a problem hiding this comment.
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?
| if (fallbackGroupId == 0u || (fallbackPayload & Flag_Mask) == Flag_Inclusive) | ||
| { | ||
| const scalar_t storeVal = Flag_Inclusive | (binop(prevReduction, currGroupReduction) << Flag_Shift); | ||
| workgroupReduction.atomicExchange(workgroupId, storeVal); |
There was a problem hiding this comment.
Be careful again, you need some sort of memory semantics to ensure correct memory barriers when different work groups access the same value atomically.
| scratchAccessor.workgroupExecutionAndMemoryBarrier(); | ||
|
|
||
| locked = sIsLocked; | ||
| scratchAccessor.workgroupExecutionAndMemoryBarrier(); |
There was a problem hiding this comment.
why workgroupExecutionAndMemoryBarrier here ?
| 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(); |
There was a problem hiding this comment.
why workgroupExecutionAndMemoryBarrier here ?
2 participants
No description provided.