PR comment changes and some more additions for subgroups

Author: kpentaris

include/nbl/builtin/hlsl/subgroup/arithmetic_portability.hlsl

@@ -5,7 +5,7 @@
 #include <nbl/builtin/hlsl/subgroup/basic_portability.hlsl>
 #endif
 
-static uint4 WHOLE_WAVE = ~0;
+static const uint4 WHOLE_WAVE = ~0; // REVIEW: Confirm this is proper placement and definition point
 
 namespace nbl
 {

include/nbl/builtin/hlsl/subgroup/arithmetic_portability_impl.hlsl

@@ -1,7 +1,7 @@
 #ifndef _NBL_BUILTIN_HLSL_SUBGROUP_ARITHMETIC_PORTABILITY_IMPL_INCLUDED_
 #define _NBL_BUILTIN_HLSL_SUBGROUP_ARITHMETIC_PORTABILITY_IMPL_INCLUDED_
 
-uint threadIndex : SV_GroupIndex; // Use HLSL terminology or follow GLSL (i.e. call them invocations instead of threads)?
+uint localInvocationIndex : SV_GroupIndex; // REVIEW: Discuss proper placement of SV_* values. They are not allowed to be defined inside a function scope, only as arguments of global variables in the shader.
 
 namespace nbl
 {
@@ -39,7 +39,7 @@ struct inclusive_scan<binops::bitwise_and>
     template<typename T>
     T operator()(const T x)
     {
-        return WaveMultiPrefixAnd(x, WHOLE_WAVE) & x; // TODO (PentaKon): Should this use the binops::bitwise_and functor?
+        return WaveMultiPrefixAnd(x, WHOLE_WAVE) & x;
     }
 };
 
@@ -268,11 +268,11 @@ struct scan_base
    static const uint HalfSubgroupSize = WaveGetLaneCount()>>1u; // TODO (PentaKon): Replace with nbl_hlsl_SubgroupSize or nbl::hlsl::subgroup::Size
    static const uint LoMask = WaveGetLaneCount()-1u; // TODO (PentaKon): Replace with nbl_hlsl_SubgroupSize
    static const uint LastWorkgroupInvocation = _NBL_HLSL_WORKGROUP_SIZE_-1; // TODO (PentaKon): Where should this be defined?
-   static const uint pseudoSubgroupInvocation = threadIndex&LoMask; // Also used in substructs
+   static const uint pseudoSubgroupInvocation = localInvocationIndex&LoMask; // Also used in substructs, thus static const
 
     static inclusive_scan<Binop,ScratchAccessor> create()
     {
-       const uint pseudoSubgroupElectedInvocation = threadIndex&(~LoMask);
+       const uint pseudoSubgroupElectedInvocation = localInvocationIndex&(~LoMask);
 
        inclusive_scan<Binop,ScratchAccessor> retval;
 
@@ -304,7 +304,7 @@ struct inclusive_scan : scan_base
 {
     static inclusive_scan<Binop,ScratchAccessor> create()
     {    
-       return scan_base<Binop,ScratchAccessor>::create(); // Is this correct?
+       return scan_base<Binop,ScratchAccessor>::create(); // REVIEW: Is this correct?
     }
 
     template<typename T, bool initializeScratch>
@@ -324,7 +324,7 @@ struct inclusive_scan : scan_base
        nbl::hlsl::subgroupBarrier();
        nbl::hlsl::subgroupMemoryBarrierShared();
        // Stone-Kogge adder
-       // it seems that lanes below <HalfSubgroupSize/step are doing useless work,
+       // (devsh): it seems that lanes below <HalfSubgroupSize/step are doing useless work,
        // but they're SIMD and adding an `if`/conditional execution is more expensive
        value = op(value,scratchAccessor.get(scanStoreOffset-1u));
        [[unroll]]

include/nbl/builtin/hlsl/subgroup/basic_portability.hlsl

@@ -5,7 +5,6 @@ namespace nbl
 {
 namespace hlsl
 {
-	// USE GLSL OR HLSL TERMINOLOGY? (Workgroup vs Threadgroup, Subgroup vs Wave, Invocation vs Thread)
 	static const uint MaxWorkgroupSizeLog2 = 11;
 	static const uint MaxWorkgroupSize = 0x1 << MaxWorkgroupSizeLog2;
 	static const uint MinSubgroupSizeLog2 = 2;
@@ -20,9 +19,50 @@ namespace hlsl
 #endif
 	static const uint MaxSubgroupSize = (0x1<<MaxSubgroupSizeLog2);
 
+	// REVIEW: No need for #ifdef check because these are always available in HLSL 2021 it seems
+	uint subgroupSize() {
+		return WaveGetLaneCount();
+	}
 
+	uint subgroupSizeLog2() {
+		return firstbithigh(subgroupSize());
+	}
+	
+	uint subgroupInvocationID() {
+		return WaveGetLaneIndex();
+	}
+
+	// REVIEW: Masks don't seem to be available in Wave Ops
+	uint64_t subgroupEqMask() { // return type for these must be 64-bit
+		// REVIEW: I think the spec says that the subgroup size can potentially go up to 128 in which case uint64 won't work properly...
+		//  Should we somehow block shaders from running if subgroup size > 64?
+		//  Use uint64_t2 ?
+		return 0x1 << subgroupInvocationID();
+	}
+	
+	uint64_t subgroupGeMask() {
+		return 0xffffffff<<subgroupInvocationID();
+	}
+	
+	uint64_t subgroupGtMask() {
+		return subgroupGeMask()<<1;
+	}
+	
+	uint64_t subgroupLeMask() {
+		return ~subgroupGtMask();
+	}
+	
+	uint64_t subgroupLtMask() {
+		return ~subgroupGeMask();
+	}
+	
+	// REVIEW: Should we define the Small/LargeSubgroupXMask and Small/LargeHalfSubgroupSizes?
 
 	// WAVE BARRIERS
+	
+	void subgroupBarrier() {
+		
+	}
 }
 }