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

devshgraphicsprogramming · 2025-04-28T14:05:00Z

include/nbl/asset/IPipeline.h

@@ -132,8 +132,10 @@ class IPipelineBase
 			Without Specialization Constants, you would have to commit
 			to a final value before the SPIR-V compilation
 		*/
+		template <bool IsMutable = false>


default template arg is probably not desired

devshgraphicsprogramming · 2025-04-28T14:07:30Z

include/nbl/asset/IPipeline.h

+			struct SSpecConstantValueImmutable
 			{
 				const void* data = nullptr;
 				//!< The byte size of the specialization constant value within the supplied data buffer.
 				uint32_t size = 0;

 				inline operator bool() const {return data&&size;}

-				auto operator<=>(const SSpecConstantValue&) const = default;
+				auto operator<=>(const SSpecConstantValueImmutable&) const = default;
+			};
+
+			struct SSPecConstantValueMutable
+			{
+				core::vector<uint8_t> data;
+				inline operator bool() const { return data.size(); }
+				auto operator<=>(const SSPecConstantValueMutable&) const = default;
 			};


I know you couldn't template specialize cause they're nested classes

But the definition is the same regardless of SShaderSpecInfo, so I'd probably hoist your SSpecConstantValue... out of this struct, ssimply so you don't get 4 separate types getting instantiated

I wonder about one thing... maybe you can just use span<const uint8_t> and vector<uint8_t> directly instead of having us invent "surplus" structs? They both have size() and empty() methods

devshgraphicsprogramming · 2025-04-28T14:26:51Z

include/nbl/video/IGPUComputePipeline.h

@@ -17,6 +17,7 @@ namespace nbl::video
 class IGPUComputePipeline : public IBackendObject, public asset::IPipeline<const IGPUPipelineLayout>
 {
        using pipeline_t = asset::IPipeline<const IGPUPipelineLayout>;
+        using spec_info_t = SShaderSpecInfo<false>;


IPipeline could have this typedef, because you just need to do = SShaderSpecInfo<is_base_of_v<IAsset,Layout>>

devshgraphicsprogramming · 2025-04-28T14:29:23Z

include/nbl/asset/IGraphicsPipeline.h

@@ -81,17 +81,17 @@ class IGraphicsPipelineBase : public virtual core::IReferenceCounted
        };
 };

-template<typename PipelineLayoutType, typename RenderpassType>
+template<typename SpecInfoType, typename PipelineLayoutType, typename RenderpassType>


the IGraphicsPipeline can work out which spec info it needs by looking if the pipeline layout inherits from asset::IAsset

and you could actually slap spec_info_t in the IPipelineBase itself

devshgraphicsprogramming · 2025-04-28T14:35:56Z

include/nbl/asset/ICPUPipeline.h

-        inline core::smart_refctd_ptr<IAsset> clone(uint32_t _depth = ~0u) const override final
-        {
-            core::smart_refctd_ptr<ICPUPipelineLayout> layout;
-            if (_depth>0u && PipelineNonAssetBase::m_layout)
-				layout = core::smart_refctd_ptr_static_cast<ICPUPipelineLayout>(PipelineNonAssetBase::m_layout->clone(_depth-1u));


you want to leave this part, and then delegate the shader cloning to the clone_impl

devshgraphicsprogramming · 2025-04-28T14:37:05Z

include/nbl/asset/ICPUPipeline.h

-                if (shader)
-                {
-                    auto stageInfo = m_stages[i].info;
-                    core::smart_refctd_ptr<IShader> newShader;
-                    if (_depth>0u)
-                    {
-                        newShader = core::smart_refctd_ptr_static_cast<IShader>(shader->clone(_depth-1u));
-                        stageInfo.shader = newShader.get();
-                    }


make a method which handles cloning a single SpecInfo in a pipeline given this clone depth, make it final and protected in this class

devshgraphicsprogramming · 2025-04-28T14:39:26Z

include/nbl/asset/IGraphicsPipeline.h

        struct SCreationParams : IPipeline<PipelineLayoutType>::SCreationParams
        {
            protected:
-                using SpecInfo = IPipelineBase::SShaderSpecInfo;


Creation Parameters should only be a thing for IGPU pipelines (you can remove one layer of inheritance.

Because CPU objects should be creatable as empty/invalid and then you mutate the shader spec infos as you wish

actually you can nerf the weird SCreationParams struct from IPipelineBase and just leave the flags (rename from SCreationParams::FLAGS to CreationFlags) in its place

frankly IPipeline can have its SCreationParams nerfed too (they only contain the pipeline layout) and moved to the correct IGPU...Pipeline

devshgraphicsprogramming · 2025-04-28T14:49:49Z

include/nbl/asset/IPipeline.h

+			using SSpecConstantValue = std::conditional_t<IsMutable, SSPecConstantValueMutable, SSpecConstantValueImmutable>;
+
 			inline SSpecConstantValue getSpecializationByteValue(const spec_constant_id_t _specConstID) const


should return by reference so it can mutate the original, meaning that you need

using SSpecConstantValue = std::conditional_t<IsMutable,core::vector<uint8_t>, span<const uint8_t>>; // non mutable getter inline std::conditional_t<IsMutable,const core::vector<uint8_t>&,span<const uint8_t>> getSpecializationByteValue(const spec_constant_id_t _specConstID) const // mutable getter, exists only if the spec info is mutable. Enable_if requires a SFINAE context hence the redundant template template<bool _IsMutable=IsMutable> requires _IsMutable inline std::enable_if_t<_IsMutable,core::vector<uint8_t>&> getSpecializationByteValue(const spec_constant_id_t _specConstID)

if you apply the suggestions from https://github.com/Devsh-Graphics-Programming/Nabla/pull/871/files#r2063742968

devshgraphicsprogramming · 2025-04-28T14:55:01Z

include/nbl/asset/ICPUComputePipeline.h

        struct SCreationParams final : IPipeline<ICPUPipelineLayout>::SCreationParams
        {
-            SShaderSpecInfo shader;
+            IPipelineBase::SShaderSpecInfo<true> shader;
        };
        static core::smart_refctd_ptr<ICPUComputePipeline> create(const SCreationParams& params)
        {
            if (!params.layout)
                return nullptr;
            auto retval = new ICPUComputePipeline(core::smart_refctd_ptr<const ICPUPipelineLayout>(params.layout));
+
            if (!retval->setSpecInfo(params.shader))
            {
                retval->drop();


get rid of the SCreationParams in the ICPU..Pipelines, and lets have a public constructor which just takes a layout (default arg nullptr) and has null spec infos

devshgraphicsprogramming · 2025-04-28T14:55:41Z

include/nbl/asset/ICPUComputePipeline.h

+            if (m_specInfo.shader)
+            {
+                SShaderSpecInfo<true> specInfo = m_specInfo;
+                if (_depth > 0u)
+                {
+                  specInfo.shader = core::smart_refctd_ptr_static_cast<IShader>(m_specInfo.shader->clone(_depth - 1u));
+                }
+                cp->setSpecInfo(specInfo);
+            }


this is common to all pipelines, can go into ICPUPipeline

like for a single spec info, the individual derived pipeline will still decide how many times and for what to call it for

devshgraphicsprogramming · 2025-04-28T15:04:41Z

include/nbl/asset/ICPUComputePipeline.h

+        inline bool setSpecInfo(const IPipelineBase::SShaderSpecInfo<true>& info)
        {
-            return stage!=hlsl::ShaderStage::ESS_COMPUTE ? (-1):0;
+          const auto specSize = info.valid();
+          if (specSize < 0) return false;
+          if (info.stage != hlsl::ESS_COMPUTE) return false;
+          m_specInfo = info;
+          return true;
        }


we need public by-reference getters for the spec info, one const and one non-const which checks/asserts if the asset is mutable

I think we can check validity of spec constants and spec info later when hashing and optionally also add a bool valid() const which tells you if the spec infos you have make sense

devshgraphicsprogramming · 2025-04-28T15:05:33Z

include/nbl/asset/ICPUGraphicsPipeline.h

 					return pipeline_base_t::SCreationParams::impl_valid(std::move(extra));
 				}
 		};
+
 		static core::smart_refctd_ptr<ICPUGraphicsPipeline> create(const SCreationParams& params)
 		{
 			// we'll validate the specialization info later when attempting to set it
-            if (!params.impl_valid([](const IPipelineBase::SShaderSpecInfo& info)->bool{return true;}))
-                return nullptr;
-            auto retval = new ICPUGraphicsPipeline(params);
-            for (const auto spec : params.shaders)
-            if (spec.shader)
-				retval->setSpecInfo(spec);
-            return core::smart_refctd_ptr<ICPUGraphicsPipeline>(retval,core::dont_grab);
+        if (!params.impl_valid([](const SShaderSpecInfo<true>& info)->bool{return true;}))
+            return nullptr;
+        auto retval = new ICPUGraphicsPipeline(params);
+        for (const auto spec : params.shaders)
+        {
+            if (spec.shader) retval->setSpecInfo(spec);
+        }
+        return core::smart_refctd_ptr<ICPUGraphicsPipeline>(retval,core::dont_grab);
 		}


again, the CPU stuff shouldn't check if stuff is valid at creation time (since its mutable

devshgraphicsprogramming · 2025-04-28T15:05:56Z

include/nbl/asset/ICPUGraphicsPipeline.h

+			return true;
+		}
+
+		SShaderSpecInfo<true> m_specInfos[GRAPHICS_SHADER_STAGE_COUNT];


for consistency use std::array

devshgraphicsprogramming · 2025-04-28T15:07:41Z

include/nbl/asset/ICPUGraphicsPipeline.h

-		inline int8_t stageToIndex(const hlsl::ShaderStage stage) const override
+		inline int8_t stageToIndex(const hlsl::ShaderStage stage) const
 		{
 			const auto stageIx = hlsl::findLSB(stage);
-			if (stageIx<0 || stageIx>=GRAPHICS_SHADER_STAGE_COUNT || hlsl::bitCount(stage)!=1)
+			if (stageIx<0 || stageIx>= GRAPHICS_SHADER_STAGE_COUNT || hlsl::bitCount(stage)!=1)
 				return -1;
 			return stageIx;
 		}
+
+    inline bool setSpecInfo(const SShaderSpecInfo<true>& info)
+		{
+			assert(isMutable());
+      const auto specSize = info.valid();
+      if (specSize<0) return false;
+			const auto stage = info.stage;
+			const auto stageIx = stageToIndex(stage);
+			if (stageIx<0) return false;
+			m_specInfos[stageIx] = info;
+			return true;
+		}


I'd change the design a bit, and allow any stage in the spec info, and we check validity later

then you can just return span<SShaderSpecInfo<true>> and span<const SShaderSpecInfo<true>> in public getters instead of cached creation params

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

devshgraphicsprogramming reviewed Apr 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework pipeline shader spec info #871

Rework pipeline shader spec info #871

kevyuu commented Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025 •

edited

Loading

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

devshgraphicsprogramming Apr 28, 2025

		using SSpecConstantValue = std::conditional_t<IsMutable, SSPecConstantValueMutable, SSpecConstantValueImmutable>;

		inline SSpecConstantValue getSpecializationByteValue(const spec_constant_id_t _specConstID) const

Rework pipeline shader spec info #871

Are you sure you want to change the base?

Rework pipeline shader spec info #871

Conversation

kevyuu commented Apr 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devshgraphicsprogramming Apr 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devshgraphicsprogramming Apr 28, 2025 •

edited

Loading