Bitonic_Sort

[CrabExtra]

Description

Testing

TODO list:

[devshgraphicsprogrammingjenkins]

[CI]: Can one of the admins verify this patch?

[Fletterio]

Define an operator= for pair, then this becomes

if(takePartnerLo)
    loPair = pLoPair;
if(takePartnerHi)
    hiPair = pHiPair;

don't worry about branching, since everything's an assignment it'll just be OpSelects under the hood

[devshgraphicsprogramming]

you can't define an operator= in HLSL none of the operators which should return references can be defined in HLSL so assignment and array indexing (as well as compound assignment)

However all structs in HLSL are trivial so can be assigned with =

[Fletterio]

This won't work without a definition for operator= for pair. Your code compiles because you're not using it rn. We want to keep this version and rewrite all the swaps to use this version using pairs.

The definition for pair, the overload for operator= and this swap method all belong in https://github.com/Devsh-Graphics-Programming/Nabla/blob/master/include/nbl/builtin/hlsl/utility.hlsl, mimicking how std::pair is in the <utility> header in cpp. Move them over there.

[devshgraphicsprogramming]

operator= can only be defined in a #ifndef __HLSL_VERSION macro block, for DXC reasons

[Fletterio]

you're using the shared memory accessor here, when you should be using an adaptor. The user only has to pass a generic sharedmem accessor, then the adaptor ensures its accesses are optimal. If changing accessor for adaptors is what breaks your code, we can look into making a specialized sharedmem adaptor for pairs.

[Fletterio]

Otherwise, we're passing the burden of writing an optimal accessor to the user, which requires the user knowing the underlying impl

[Fletterio]

Hold this one until Matt replies because the adaptor needs to change here

[devshgraphicsprogramming]

Otherwise, we're passing the burden of writing an optimal accessor to the user, which requires the user knowing the underlying impl

yep

[Fletterio]

Feels overkill to do this. Instead of having to go over half the array to swap it around just make the E=2 struct specialization take an ascending parameter like you did for the subgroup sort. Set it to true by default, and in the calls above you need in this E > 2 struct pass ascending = !(WorkgroupID & 1) to ensure every even workgroup is sorted ascendingly and every odd workgroup is sorted descendingly. It's the same you did for the subgroup case.

[devshgraphicsprogramming]

doesn't seem to be done and is important, but maybe lets try the Subgroup Transpose trick

[devshgraphicsprogramming]

where's the SubgroupSizeLog2 ?

[devshgraphicsprogramming]

needs to be compile time constant from config like the FFT

[devshgraphicsprogramming]

you also want to handle "multiple items per thread" in the subgroup sort

[devshgraphicsprogramming]

write a TODO to make a struct which can be specialized by declaring a PoT sorting network within a thread

[devshgraphicsprogramming]

example

template<typename KeyValue, uint32_t Log2N>
struct SortingNetwork
{
   void operator()(const bool ascending, NBL_REF_ARG(array<KeyValue,0x1u<<Log2N>) data, NBL_CONST_REF_ARG(Comparator) comp);
};

and then you can partially specialize it for all Log2N from 1 to 3 or 4

[devshgraphicsprogramming]

don't do everything for pair<Key,Value> but a KeyValue on which get<0,KeyValue>(kv) and get<0,KeyValue>(kv) can be called (instead of .first and .second).

And then you can trivially allow pair<K,V> be used as KeyValue

Why? You will have the option of "packing" the key or value into the would-be padding bits of each other (when SoA isn't possible or beneficial) and doing all sorts of other tricks

[devshgraphicsprogramming]

furthemore to allow working on more than 2 elements per thread the signature should really just be bool, NBL_REF_ARG(KeyValue) data[ElementsPerThread])

can do NBL_REF_ARG(array<U,ElementsPerThread>) if DXC gives you pain on array types

[devshgraphicsprogramming]

btw its fine to not rewrite the function and static_assert(ElementsPErThread==2) for now

or just provide the partial spec for ElementsPerThread==2 (since static assert gives trouble in DXC SPIR-V backend) but then you need to work it into a struct functor

[devshgraphicsprogramming]

I've just talked with @Fletterio and we figured out how to trade subgroup variables to not have to perform redundant comparisons, this means 4-way Key-Value exchanges per thread won't be necessary and this function can disappear

[devshgraphicsprogramming]

there's a faster way to write this loSelfSmaller==takeLarger, look at a T/F table of your current expression

[devshgraphicsprogramming]

are you sure this is correct?
#940 (comment)

if I myself am smaller, then comp which is < will return true, but I will swap with partner when takeLarger is true

did you mess up variable naming here?

[devshgraphicsprogramming]

there's a faster way to write this hiSelfSmaller==takeLarger, look at a T/F table of your current expression

[devshgraphicsprogramming]

why not write the loop as for (uint32_t stride=1u<<stage; stride; stride=stride>>1) and forget the pass ?

[devshgraphicsprogramming]

bitonic_sort::BitonicSort... is a tautology, drop the BitonicSort prefix from the names

[devshgraphicsprogramming]

transform into

template<typename KeyValue, uint32_t Log2N>
struct LocalPasses
{
   void operator()(const bool ascending, NBL_REF_ARG(array<KeyValue,0x1u<<Log2N>) data, NBL_CONST_REF_ARG(Comparator) comp);
};

and make your current implementation a partial spec for LocalPasses<KeyValue,2>

[devshgraphicsprogramming]

actually I'd ask for an even cooler feature, would be nice if stage<=sortSizeLog2 and __call had a last argument with a default const uint32_t sortSizeLog2=Config::SubgroupSizeLog2

Why? Because it would be super useful for things like sorting arrays which are smaller than what a subgroup can process (so we can pack multiple arrays to be sorted independently into a single subgroup)

[devshgraphicsprogramming]

the const bool bitonicAscending = (stage == subgroupSizeLog2) ? would have to change to const bool bitonicAscending = (stage == sortSizeLog2 ) ?

[devshgraphicsprogramming]

here's a problem with subgroupShuffle it only works on scalars and vectors.

Now why would we use a Comparison Based Sort over a Counting Sort?

Because the Key is really long in relation to number of elements to sort OR only supports custom comparison and can't be represented as a ordered number.

Therefore its most useful whenever the Key is a struct.

You need to document and require (conceptualize) that your KeyValue needs to have a shuffle_type typedef which is a vector<fundamental_type,N> and the KeyValue can be static_cast-ed to and from shuffle_type, you won't be able to just subgroupShuffle a pair<Key,Value> or anything similar

[devshgraphicsprogramming]

Actually drop the static_cast if we had a stateful converter then we could avoid doing silly things

Why are we even trading/shuffling a full value type? when all that we need to do is give each Kev a surrogate Value which is just subgroupInvocationIndex which tells us how to shuffle at the very end.

[devshgraphicsprogramming]

want a get and a get_helper struct in this header, plus a particla spec for pair in this header

[devshgraphicsprogramming]

this won't unroll, but this will

for (uint32_t baseVirtualThread=0; baseVirtualThread<HalfTotalElements; baseVirtualThread+=WorkgroupSize)
{
  const uint32_t virtualThreadID = baseVirtualThread+threadID;

the compiler now see that the loop condition is invariant (otherwise you have a dep on threadID and netiher DXC or SPIR-V opt can do the long and ardous analysis to see if your workgroup count is a multiple and the loop condition is actually uniform for whole group always)

[devshgraphicsprogramming]

already pointed it out Devsh-Graphics-Programming/Nabla-Examples-and-Tests#209 (comment)

[devshgraphicsprogramming]

you should just require that _WorkgroupSizeLog2>SubgroupSizeLog2 I see you've hard coded an assumption that Subgroups are Size 32, the subgroup size needs to come into the config too

[devshgraphicsprogramming]

There should also be a general TotalElementsLog2>SubgroupSizeLog2+ElementsPerInvocationLog2 then this tells us our VirtualWorkgroupSizeLog2

[devshgraphicsprogramming]

SharedMemorySize should come from outside, and it must be as big as sizeof(KeyValue::workgroup_type)<<WorkgroupSizeLog2

the idea is that if we have more shared memory, then we issue less barriers by:

1. exchanging more items per barrier (up to 0x1u<<(WorkgroupSizeLog2+ElementPerInvocationLog2))
2. cycling through offsets within the shared memory to barrier every so often (every time you wrap around)

[devshgraphicsprogramming]

@Fletterio I think device_capabilities aren't really needed for anything here or the FFT because we baiscally require Shuffle operations, unlike where for the Prefix sum we use them to decide between native subgroupAdd and a thing made with subgroup shuffles

[devshgraphicsprogramming]

@Fletterio an XOR shuffle of a struct (single accessor) within a workgroup should be commonalized somewhere cause its used in FFT and here

[Fletterio]

I'll try making a generic version but memory layouts fuck me up a bit. I'll iterate on a version.

[devshgraphicsprogramming]

oh yeah with shuffling KeyValue and leaving the SoA/AoS bank-conflict decomposition to userspace Accessor allows us to use the workgroup shuffleXoR #940 (comment)

its probably worth extending that thing to handle Virtual Workgroups (when you have more shared memory and can do multiple shuffles without a barrier in the middle

[devshgraphicsprogramming]

TotalElements should already be passed as Log2

[devshgraphicsprogramming]

put the barrier at the end of the loop then you don't need if(sub) and the barrier after the loop

[devshgraphicsprogramming]

oh btw the Spec doesn't say that LocalInvocationIndex = SubgroupID*SubgroupSize+SubgroupInvocationID

This is why in such shaders you need to always use

Nabla/include/nbl/builtin/hlsl/workgroup/basic.hlsl

Line 26 in b7f3dfb

uint16_t SubgroupContiguousIndex()

instead of gl_LocalInvocationID or gl_LocalInvocationIndex

[devshgraphicsprogramming]

there's actually a GLSL builtin for this

[devshgraphicsprogramming]

ElementsPerThread should be the number of elements each thread operates on, the multiplier for Virtual Threading (what your current ElementsPerThread acts as) should be derived from TotalElementsLog2-WorkgroupSizeLog2-ElementsPerThreadLog2

[devshgraphicsprogramming]

in view of #943 (comment) you could actually count VirtualSubgroups

[devshgraphicsprogramming]

it would be cool derive this as TotalElementsLog2-SubgroupSizeLog2-ElementsPerThreadLog2 this would allow us to do workgroup partitioned sorts, eg:

• sort 4x256 with a 512 workgroup elements per thread 2

[devshgraphicsprogramming]

comments throughout code needed about this stuff #940 (comment)

[devshgraphicsprogramming]

its declaraed in a tautologic namespace, as @Fletterio mentioned we don't want bitonic_sort::BitonicSort

[devshgraphicsprogramming]

you're doing double the work... unlike subgroups where an invocation can't write into its partner's register. here you can definitely have one thread read both items, and write both items

[devshgraphicsprogramming]

can skip the StructureOfArrays in this code, more useful to use it in userspace , see #943 (comment)