Rework pipeline shader spec info #871
Conversation
devshgraphicsprogramming
changed the base branch from
master
to
stagesless_shaders
include/nbl/asset/ICPUPipeline.h
Outdated
| 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(); | ||
| } |
There was a problem hiding this comment.
make a method which handles cloning a single SpecInfo in a pipeline given this clone depth, make it final and protected in this class
There was a problem hiding this comment.
my bad, make it a method of the SpecInfo
| 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; | ||
| } |
There was a problem hiding this comment.
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
| explicit ICPUComputePipeline(const ICPUPipelineLayout* layout): | ||
| base_t(core::smart_refctd_ptr<ICPUPipelineLayout>(layout)) | ||
| {} | ||
|
|
||
| static core::smart_refctd_ptr<ICPUComputePipeline> create(const ICPUPipelineLayout* layout) |
There was a problem hiding this comment.
have one, either the create factory or a public constructor, make the constructor private
| return core::smart_refctd_ptr<ICPUComputePipeline>(retval,core::dont_grab); | ||
| } | ||
|
|
||
| inline core::smart_refctd_ptr<IAsset> clone(uint32_t _depth = ~0u) const override final | ||
| inline base_t* clone_impl(core::smart_refctd_ptr<const ICPUPipelineLayout>&& layout, uint32_t depth) const override final |
There was a problem hiding this comment.
I don't think an override can change the return type of a function, it needs to return core::smart_refctd_ptr<IAsset>
also clone_impl spawns a new object which needs to be reference counted, therefore it needs to return a smart pointer and not a raw pointer
ok I see that clone_impl is a new function you added to ICPUPipeline, but I would not prioritise keeping code DRY and abstracting away a smart pointer assign, over readability so please return smartpointer
There was a problem hiding this comment.
btw clone_impl shall be private or protected
There was a problem hiding this comment.
also you can mark the whole ICPUComputePipeline class as final
| enum class FLAGS : uint64_t | ||
| { | ||
| NONE = 0, // disallowed in maintanance5 | ||
| DISABLE_OPTIMIZATIONS = 1<<0, | ||
| ALLOW_DERIVATIVES = 1<<1, | ||
|
|
||
| // I can just derive this | ||
| //DERIVATIVE = 1<<2, | ||
| public: | ||
| enum class CreationFlags : uint64_t |
There was a problem hiding this comment.
you want an alias of FLAGS = CreationFlags so you con't need to refactor lots of code
include/nbl/asset/ICPUPipeline.h
Outdated
| struct SSpecConstantValue | ||
| { | ||
| core::vector<uint8_t> data; | ||
| inline operator bool() const { return data.size(); } | ||
| inline size_t size() const { return data.size(); } | ||
| }; | ||
|
|
||
| inline SSpecConstantValue* getSpecializationByteValue(const spec_constant_id_t _specConstID) | ||
| { | ||
| const auto found = entries.find(_specConstID); | ||
| if (found != entries.end() && bool(found->second)) return &found->second; | ||
| else return nullptr; | ||
| } |
There was a problem hiding this comment.
why not just alias/typedef SSpecConstantValue = cover::vector<uint8_t> ?
| 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; |
There was a problem hiding this comment.
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)
include/nbl/asset/ICPUPipeline.h
Outdated
| core::smart_refctd_ptr<ICPUPipelineLayout> layout; | ||
| if (_depth>0u && getLayout()) | ||
| layout = core::smart_refctd_ptr_static_cast<ICPUPipelineLayout>(getLayout->clone(_depth-1u)); | ||
|
|
||
| auto* newPipeline = clone_impl(std::move(layout), _depth); | ||
|
|
There was a problem hiding this comment.
geenerally speaking you can return nullptr right away if there isn't a valid layout
include/nbl/video/IGPUPipeline.h
Outdated
| // Common Base class for pipelines | ||
| template<typename PipelineNonAssetBase> | ||
| class IGPUPipeline : public IBackendObject, public PipelineNonAssetBase, public IGPUPipelineBase |
There was a problem hiding this comment.
same comment about PipelineNonAssetBase
There was a problem hiding this comment.
should probably be called PipelineNonBackendObjectBase and have a constraint like I suggested
| // If you set this, then we don't take `basePipelineIndex` into account, the pointer takes precedence | ||
There was a problem hiding this comment.
don't touch my comment
| @@ -51,7 +50,7 @@ class IGPUComputePipeline : public IBackendObject, public asset::IPipeline<const | |||
| if (dataSize<0) | |||
| return {}; | |||
| // https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkComputePipelineCreateInfo.html#VUID-VkComputePipelineCreateInfo-stage-00701 | |||
| if (!layout || shader.stage!=hlsl::ShaderStage::ESS_COMPUTE) | |||
| if (!layout) | |||
| return {}; | |||
|
|
|||
| uint32_t count = 0; | |||
There was a problem hiding this comment.
don't we already have the entries validation somwhere ?
There was a problem hiding this comment.
I swear I just say it while reviewing
| inline std::span<const spec_info_t> getShaders() const {return {&shader,1}; } | ||
| inline std::span<const SShaderSpecInfo> getShaders() const {return {&shader,1}; } |
There was a problem hiding this comment.
what do we need getShaders in GPU Pipeline Creation parameters for ?
| const auto stageIx = hlsl::findLSB(stage); | ||
| if (stageIx < 0 || stageIx >= GRAPHICS_SHADER_STAGE_COUNT || hlsl::bitCount(stage)!=1) | ||
| return -1; | ||
| return stageIx; | ||
| } | ||
|
|
||
| inline const SCachedCreationParams& getCachedCreationParams() const {return m_params;} | ||
| static inline hlsl::ShaderStage indexToStage(const int8_t index) | ||
| { | ||
| if (index < 0 || index > GRAPHICS_SHADER_STAGE_COUNT) | ||
| return hlsl::ShaderStage::ESS_UNKNOWN; | ||
| return static_cast<hlsl::ShaderStage>(hlsl::ShaderStage::ESS_VERTEX + index); | ||
| } |
There was a problem hiding this comment.
these stage-to-index functions we don't need anymore, CPU class and GPU creation params will have 5 separate named members each
There was a problem hiding this comment.
I see that it makes your life easier in ICPUGraphicsPipeline, but that sort of mapping shouldn't be used in IGPUGraphicsPipeline::SCreationParams, so you can move these static methods to ICPUGraphicsPipeline
|
|
||
| inline const renderpass_t* getRenderpass() const {return m_renderpass.get();} | ||
| static inline bool isValidStagePresence(const core::bitflag<hlsl::ShaderStage>& stagePresence, E_PRIMITIVE_TOPOLOGY primitiveType) |
There was a problem hiding this comment.
maybe name the method hasRequiredStages
| @@ -61,8 +81,9 @@ class IGPURayTracingPipeline : public IBackendObject, public asset::IRayTracingP | |||
| return retval; | |||
| } | |||
|
|
|||
| inline std::span<const asset::IPipelineBase::SShaderSpecInfo> getShaders() const { return shaders; } | |||
| inline std::span<const spec_info_t> getShaders() const { return shaders; } | |||
There was a problem hiding this comment.
the creation params shouldn't need a getter, you need 6 different members (5 spans and 1 raygen)
| explicit ICPUGraphicsPipeline(const ICPUPipelineLayout* layout) | ||
| : base_t(layout, {}, {}) | ||
| {} | ||
|
|
||
| static core::smart_refctd_ptr<ICPUGraphicsPipeline> create(const ICPUPipelineLayout* layout) | ||
| { |
There was a problem hiding this comment.
again, constructor, private, create factory public
| inline virtual bool valid() const override final | ||
| { | ||
| if (!m_layout) return false; |
There was a problem hiding this comment.
maybe valid() should be in IAsset and ICPUPipeline should already override and check the layout presence
| inline virtual std::span<SShaderSpecInfo> getSpecInfo(hlsl::ShaderStage stage) override final | ||
| { | ||
| const auto stageIndex = stageToIndex(stage); | ||
| if (isMutable() && stageIndex != -1) | ||
| { | ||
| return { &m_specInfos[stageIndex], 1 }; | ||
| } | ||
| return {}; |
There was a problem hiding this comment.
you also need a const flavour which doesn't check for mutability (maybe implement non-const in terms of const + isMutable() check)
There was a problem hiding this comment.
basically you got the DRY implementation backwards in ICPUPipeline
| inline virtual bool valid() const override final | ||
| { | ||
| return m_specInfo.valid(); | ||
| } |
There was a problem hiding this comment.
CPUGraphicsPipeline checks the layout, you don't check here ?
include/nbl/asset/ICPUPipeline.h
Outdated
| } | ||
|
|
||
| virtual bool valid() const = 0; |
There was a problem hiding this comment.
move to IAsset, I'm thinking more and more classes will need this
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: