Rework pipeline shader spec info #871

kevyuu · 2025-04-28T13:05:13Z

Rework SShaderSpecInfo for ICPUPipeline to be more mutable:
IPipelineBase::SShaderSpecInfo should be templated on a boolean being mutable or not, such that members are conditional_t, and for CPU:

shader is smart pointer
entryPoint is a string and not a string view
entries are an actual unordered_map instead of a pointer to one (this is bit complicated cause you need to make IPipelineBase::SShaderSpecInfo::SSpecConstantValue hold a vector of uint8_t instead of a const void* + size

include/nbl/asset/IPipeline.h

include/nbl/video/IGPUComputePipeline.h

include/nbl/asset/IGraphicsPipeline.h

include/nbl/asset/ICPUPipeline.h

include/nbl/asset/IGraphicsPipeline.h

include/nbl/asset/IPipeline.h

include/nbl/asset/ICPUComputePipeline.h

include/nbl/asset/ICPUGraphicsPipeline.h

include/nbl/asset/ICPUComputePipeline.h

include/nbl/asset/IPipeline.h

include/nbl/asset/ICPUPipeline.h

devshgraphicsprogramming · 2025-05-05T15:15:09Z

include/nbl/asset/ICPUPipeline.h

+            core::smart_refctd_ptr<IShader> shader = nullptr;
+            std::string entryPoint = "";
+            IPipelineBase::SUBGROUP_SIZE requiredSubgroupSize : 3 = IPipelineBase::SUBGROUP_SIZE::UNKNOWN;	//!< Default value of 8 means no requirement
+            uint8_t requireFullSubgroups : 1 = false;


hmm maybe we should have a ICPUComputePipeline::SShaderSpecInfo : ICPUPipeline::SShaderSpecInfo to enforce even more strong typing (no requireFullSubgroup in pipelines not using the compute or mesh stage)

ok I see that since you saw that compute only has one stage, you hoisted the requireFullSubgroup into the creation parameters, which is acceptable, unfortunately you took the requiredSubgroupSize with it which was not supposed to happen

include/nbl/asset/ICPUPipeline.h

devshgraphicsprogramming · 2025-05-13T15:24:42Z

include/nbl/asset/ICPUGraphicsPipeline.h

            return {};
        }

+
        inline virtual bool valid() const override final
        {
            if (!m_layout) return false;


layout valid() check needed too?

devshgraphicsprogramming · 2025-05-13T15:26:12Z

include/nbl/asset/ICPUComputePipeline.h

+        inline core::smart_refctd_ptr<base_t> clone_impl(core::smart_refctd_ptr<const ICPUPipelineLayout>&& layout, uint32_t depth) const override final
        {


cannot be publid

devshgraphicsprogramming · 2025-05-13T15:27:37Z

include/nbl/asset/ICPUComputePipeline.h

        inline virtual bool valid() const override final
        {
+            if (!m_layout) return false;


layout valid check?

devshgraphicsprogramming · 2025-05-13T15:28:12Z

include/nbl/asset/ICPUComputePipeline.h

+        inline virtual std::span<const SShaderSpecInfo> getSpecInfo(hlsl::ShaderStage stage) const override final
        {
            if (stage==hlsl::ShaderStage::ESS_COMPUTE && isMutable())
                return {&m_specInfo,1};


const method does not need to check isMutable(), the non-const (which I take is implemented in the base class) does

devshgraphicsprogramming · 2025-05-13T15:31:45Z

include/nbl/asset/ICPUPipeline.h

-        virtual std::span<SShaderSpecInfo> getSpecInfo(const hlsl::ShaderStage stage) = 0;
-        inline std::span<const SShaderSpecInfo> getSpecInfo(const hlsl::ShaderStage stage) const
-        {
-            return getSpecInfo(stage);
-        }
+        virtual std::span<const SShaderSpecInfo> getSpecInfo(const hlsl::ShaderStage stage) const = 0;


where's our murable variant?

ok I see its in ICPUPipeline, but why not here?

isMutable is a method of ICPUPipeline, not ICPUPipelineBase

devshgraphicsprogramming · 2025-05-13T15:34:38Z

include/nbl/asset/ICPUPipeline.h

-            }
-            return newSpecInfo;
+            if (!isMutable()) return {};
+            const auto specInfo = static_cast<const this_t*>(this)->getSpecInfo(stage);


const_cast not static cast, you're not going up or down the hierarchy

devshgraphicsprogramming · 2025-05-13T15:35:27Z

include/nbl/asset/IPreHashed.h

-				size_t childCount = 0;
-				size_t childrenVisited = 0;
+				const IAsset* asset;
+				core::unordered_set<const IAsset*> unvisitedChilds;


grammar nitpick, the plural of child is children

there's a more efficient way to do this, you can keep one unordered set (the alreadyVisited) and instead of pushing the asset + the returned unordered_map onto a stack, you quickly iterate the returned unordered_map and push their contents onto the stack if they're not in alreadyVisited

devshgraphicsprogramming · 2025-05-13T15:44:41Z

include/nbl/video/IGPURayTracingPipeline.h

+              SKIP_BUILT_IN_PRIMITIVES = 1<<12,
+              SKIP_AABBS = 1<<13,
+              NO_NULL_ANY_HIT_SHADERS = 1<<14,
+              NO_NULL_CLOSEST_HIT_SHADERS = 1<<15,
+              NO_NULL_MISS_SHADERS = 1<<16,
+              NO_NULL_INTERSECTION_SHADERS = 1<<17,
+              ALLOW_MOTION = 1<<20,


I think ICPURayTracingPipeline also needs to know about these flags and they must be settable on it ( so the cached creation params have the flags then)

esp the SKIP and ALLOW_MOTION, the NO_NULL we can have a debate about

devshgraphicsprogramming · 2025-05-13T15:46:25Z

include/nbl/asset/IComputePipeline.h

+    // Nabla requires device's reported subgroup size to be between 4 and 128
+    enum class SUBGROUP_SIZE : uint8_t
+    {
+      // No constraint but probably means `gl_SubgroupSize` is Dynamically Uniform
+      UNKNOWN = 0,
+      // Allows the Subgroup Uniform `gl_SubgroupSize` to be non-Dynamically Uniform and vary between Device's min and max
+      VARYING = 1,
+      // The rest we encode as log2(x) of the required value
+      REQUIRE_4 = 2,
+      REQUIRE_8 = 3,
+      REQUIRE_16 = 4,
+      REQUIRE_32 = 5,
+      REQUIRE_64 = 6,
+      REQUIRE_128 = 7
+    };
+
+    struct SCachedCreationParams final
+    {
+        SUBGROUP_SIZE requiredSubgroupSize : 3 = SUBGROUP_SIZE::UNKNOWN;	//!< Default value of 8 means no requirement


nooo, you can require a subgroup size on any shader! needs to go back into ShaderSpecInfo

its only requireFullSubgroups thats a compute-specific thing

devshgraphicsprogramming · 2025-05-14T09:20:11Z

include/nbl/asset/IPipeline.h

-      // Nabla requires device's reported subgroup size to be between 4 and 128
-      enum class SUBGROUP_SIZE : uint8_t
-      {
-        // No constraint but probably means `gl_SubgroupSize` is Dynamically Uniform
-        UNKNOWN = 0,
-        // Allows the Subgroup Uniform `gl_SubgroupSize` to be non-Dynamically Uniform and vary between Device's min and max
-        VARYING = 1,
-        // The rest we encode as log2(x) of the required value
-        REQUIRE_4 = 2,
-        REQUIRE_8 = 3,
-        REQUIRE_16 = 4,
-        REQUIRE_32 = 5,
-        REQUIRE_64 = 6,
-        REQUIRE_128 = 7
-      };


should have stayed here

devshgraphicsprogramming · 2025-05-14T09:22:49Z

include/nbl/asset/ICPUPipeline.h

            core::smart_refctd_ptr<ICPUPipelineLayout> layout;
-            if (_depth>0u && getLayout()) 
+            if (_depth > 0u) 
              layout = core::smart_refctd_ptr_static_cast<ICPUPipelineLayout>(getLayout->clone(_depth-1u));


syntax error, no () on the getLayout ?

devshgraphicsprogramming · 2025-05-14T09:36:13Z

include/nbl/asset/ICPUGraphicsPipeline.h

-        inline base_t* clone_impl(core::smart_refctd_ptr<const ICPUPipelineLayout>&& layout, uint32_t depth) const override final
+        inline core::smart_refctd_ptr<base_t> clone_impl(core::smart_refctd_ptr<const ICPUPipelineLayout>&& layout, uint32_t depth) const override final


again, cannot be public

devshgraphicsprogramming · 2025-05-14T09:47:22Z

include/nbl/video/IGPUGraphicsPipeline.h

+                    if (!specInfo.shader) return false;
+                    stagePresence != stage;
+                    return true;
+                };
+                if (!processSpecInfo(vertexShader)) return false;


you're returning false if a shader is missing, its only the vertex shader that's required, I think you should replace

if (!specInfo.shader) return false; stagePresence != stage; return true;

with

if (specInfo.shader) { if (specInfo.valid()) // does this check spec constants being correctly set up etc? return false; stagePresence |= stage; } return false;

you also have a typo with != having the not operator instead of |

devshgraphicsprogramming · 2025-05-14T10:17:10Z

include/nbl/video/IGPURayTracingPipeline.h

+                          const auto stage = info.stage;
+                          if ((stage & ~hlsl::ShaderStage::ESS_ALL_RAY_TRACING) != 0)
+                            return false;
+                          if (!std::has_single_bit<std::underlying_type_t<hlsl::ShaderStage>>(stage))
+                            return false;


stage is now implied from the member position and doesn't need to be checked

devshgraphicsprogramming · 2025-05-14T10:17:37Z

include/nbl/video/IGPURayTracingPipeline.h

+                          // every shader must not be null. use SIndex::Unused to represent unused shader.
+                          return false;


now nullptr shader stages will imply and Unused index

you only need to check that raygen is present and valid

devshgraphicsprogramming · 2025-05-14T10:18:13Z

include/nbl/video/IGPURayTracingPipeline.h

+              return impl_valid([](const SShaderSpecInfo& info)->bool
+              {
+                if (!info.valid())
+                  return false;
+                return false;
+              });


thats just repeating over and over, I don't think we need callbacks to check shader stages anymore

devshgraphicsprogramming · 2025-05-14T10:18:56Z

include/nbl/video/IGPURayTracingPipeline.h

+                  auto getShaderStage = [this](size_t index) -> hlsl::ShaderStage
+                  {
+                      return shaders[index].stage;
+                  };
+
+                auto isValidShaderIndex = [this, getShaderStage](size_t index, hlsl::ShaderStage expectedStage, bool is_unused_shader_forbidden) -> bool
+                  {
+                    if (index == SShaderGroupsParams::SIndex::Unused)
+                      return !is_unused_shader_forbidden;
+                    if (index >= shaders.size())
+                      return false;
+                    if (getShaderStage(index) != expectedStage)
+                      return false;
+                    return true;
+                  };
+
+                if (!isValidShaderIndex(shaderGroups.raygen.index, hlsl::ShaderStage::ESS_RAYGEN, true))
+                {
+                  return false;
+                }


all of this is stale code and needs to go

devshgraphicsprogramming · 2025-05-14T10:19:16Z

include/nbl/video/IGPURayTracingPipeline.h

+                for (const auto& shaderGroup : shaderGroups.hits)
+                {
+                  // https://docs.vulkan.org/spec/latest/chapters/pipelines.html#VUID-VkRayTracingPipelineCreateInfoKHR-flags-03470
+                  if (!isValidShaderIndex(shaderGroup.anyHit, 
+                    hlsl::ShaderStage::ESS_ANY_HIT,
+                    bool(flags & FLAGS::NO_NULL_ANY_HIT_SHADERS)))
+                    return false;
+
+                  // https://docs.vulkan.org/spec/latest/chapters/pipelines.html#VUID-VkRayTracingPipelineCreateInfoKHR-flags-03471
+                  if (!isValidShaderIndex(shaderGroup.closestHit, 
+                    hlsl::ShaderStage::ESS_CLOSEST_HIT,
+                    bool(flags & FLAGS::NO_NULL_CLOSEST_HIT_SHADERS)))
+                    return false;
+
+                  if (!isValidShaderIndex(shaderGroup.intersection, 
+                    hlsl::ShaderStage::ESS_INTERSECTION,
+                    false))
+                    return false;
+                }
+
+                for (const auto& shaderGroup : shaderGroups.misses)
+                {
+                  if (!isValidShaderIndex(shaderGroup.index, 
+                    hlsl::ShaderStage::ESS_MISS, 
+                    false))
+                    return false;
+                }
+
+                for (const auto& shaderGroup : shaderGroups.callables)
+                {
+                  if (!isValidShaderIndex(shaderGroup.index, hlsl::ShaderStage::ESS_CALLABLE, false))
+                    return false;
+                }


rewrite needed, correct?

devshgraphicsprogramming · 2025-05-14T10:20:56Z

include/nbl/video/IGPURayTracingPipeline.h

@@ -15,6 +15,147 @@ class IGPURayTracingPipeline :  public IGPUPipeline<asset::IRayTracingPipeline<c
        using pipeline_t = asset::IRayTracingPipeline<const IGPUPipelineLayout>;

    public:
+        struct SCreationParams


why do you have both this and SCreationParams below?

include/nbl/asset/ICPURayTracingPipeline.h

devshgraphicsprogramming · 2025-05-14T10:22:35Z

include/nbl/asset/ICPURayTracingPipeline.h

+            return core::smart_refctd_ptr<ICPURayTracingPipeline>(retval,core::dont_grab);
+        }
+
+        inline core::smart_refctd_ptr<base_t> clone_impl(core::smart_refctd_ptr<const ICPUPipelineLayout>&& layout, uint32_t depth) const override final


cannot be public

devshgraphicsprogramming · 2025-05-14T10:25:09Z

include/nbl/asset/ICPURayTracingPipeline.h

+        inline virtual std::span<const SShaderSpecInfo> getSpecInfo(hlsl::ShaderStage stage) const override final
+        {
+            switch (stage) 
+            {
+                case hlsl::ShaderStage::ESS_RAYGEN:
+                  return { &m_raygen, 1 };
+                case hlsl::ShaderStage::ESS_MISS:
+                  return m_misses;
+                case hlsl::ShaderStage::ESS_ANY_HIT:
+                  return m_hitGroups.anyHits;
+                case hlsl::ShaderStage::ESS_CLOSEST_HIT:
+                  return m_hitGroups.closestHits;
+                case hlsl::ShaderStage::ESS_INTERSECTION:
+                  return m_hitGroups.intersections;
+                case hlsl::ShaderStage::ESS_CALLABLE:
+                  return m_callables;
+
+            }
+            return {};
+        }


you need a mutable getter of the vector<ShaderSpecInfo>& itself because otherwise we can't add/remove elements after creation only mutate ones which are already there

devshgraphicsprogramming · 2025-05-14T10:29:03Z

include/nbl/asset/ICPURayTracingPipeline.h

+            // TODO(kevinyu): Fix this temporary dummy code
+            return true;


If we're on the same page, then this is baiscally checking a layout and raygen shaders are there

All other stages are optional, but if they are present, they need to be checked for validity (spec constants and stuff)

Remember that we can patch NO_NULL flags in the asset converter if we detect incompatibility (missing stages), those will be in our patch.

kevyuu added 3 commits April 28, 2025 19:59

Implement mutable shader spec info

dc41722

Rework IGPUGraphicsPipeline

7fe3431

Rework IGPUComputePipeline.h

8dbe9c7

devshgraphicsprogramming changed the base branch from master to stagesless_shaders April 28, 2025 14:03