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

[Fletterio]

delete all of these and only use the versions that use pairs. It's more consistent

[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.

[Fletterio]

With the definition for the swap below, this just becomes

if (doSwap)
    swap(loPair, hiPair);

[Fletterio]

Remove this

[Fletterio]

All of this belongs in https://github.com/Devsh-Graphics-Programming/Nabla/blob/master/include/nbl/builtin/hlsl/utility.hlsl. Move the code over there and remove this file.

[Fletterio]

mergeStage(..., loPair, hiPair)

[Fletterio]

__call(ascending, loPair, hiPair)

[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

[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]

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]

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

[devshgraphicsprogramming]

there's a faster way to write this loSelfSmaller==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.