Vulkan: Add: Specialization constants #116952
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#116952
Loading…
Reference in New Issue
No description provided.
Delete Branch "(deleted):vk_specialization_constants"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Add implementation for using specialization constants.
Benefits of using this feature:
ShaderModules are reusable
Uniformly specialized for each hardware
Easy to support scalability
Optimizing "shader programs" (e.g. extending "for" statements)
Current assumptions:
It may be useful to know whether specialization constants change when a pipeline is created.
This is because the number of Pipeline generation times can be reduced.
Therefore, its value is maintained.
I quickly glanced over this. Will do a more in depth review tomorrow.
The coding pattern that we use is that all specialization constants needs to be set before a shader is bound. Has to do with how OpenGL would emulate this feature. This will also make the implementation easier. But the OpenGL might need to land first to get advantage from that.
Taking this into account the VkSpecializationInfo can be constructed in
VKShader::bind
and when needed it can just be referenced.Added support for changes to OpenGL. Thank you for your review.
@ -38,3 +39,3 @@
}
VKPipeline VKPipeline::create_compute_pipeline(
VKPipeline* VKPipeline::create_compute_pipeline(
Better use
std::unique_ptr<VKPipeline>
as return type. Unsure if this change is actually needed.@ -1021,1 +1036,3 @@
<< "u)\n";
case Type::FLOAT: {
/* Use uint representation to allow exact same bit pattern even if NaN. */
float f = *reinterpret_cast<float *>(const_cast<uint32_t *>(&sc.default_value.u));
If this is needed and
.fl
cannot be used, we should comment why,@ -1407,0 +1429,4 @@
VkSpecializationInfo null = {};
return null;
}
static Vector<VkSpecializationMapEntry> entries;
Perhaps better to cache this inside the VKBackend containing a fixed number of values (8). Assert when more constants are needed than allocated.
and use
constants.values.size()
formapEntryCount
@ -1407,0 +1443,4 @@
/* const void *pData */ constants.values.data()};
};
const VkSpecializationInfo VKShader::specialzation_ensure()
spelling mistake
@ -33,2 +33,2 @@
VKPipeline pipeline_;
std::unique_ptr<VKPipeline> pipeline_;
bool vk_specialtization_dirty_ = false;
spelling
There have been a few changes.
Please review again.
@ -464,12 +464,10 @@ void GPU_shader_bind(GPUShader *gpu_shader)
shader->bind();
GPU_matrix_bind(gpu_shader);
Shader::set_srgb_uniform(gpu_shader);
shader->constants.is_dirty = false;
OpenGL requires this. changing this will break the opengl implementation.
VKPipeline stores the latest specialization constant info as an attribute and mark itself dirty.
When a draw/dispatch command is done the pipeline can be reconstructed based on this dirty flag.
I think this will make the implementation easier to understand and follow the intent of the original design of specialization constants.
I see. This seems to lead to trying to cache the pipeline using specialization constants. First of all, this change doesn't prevent it. And you said that OpenGL would break, but the logic behind that is a little unclear to me. And I missed one change. (This is my mistake, but this oversight is not required). The shader::bind function always calls the
Program_get()
function when theis_dirty
flag is True, so as long as it is set to false there, the system logic remains unchanged.@ -197,1 +195,3 @@
pipeline.update_push_constants(*this);
Vector<VkSpecializationMapEntry> specialization_map_entries;
shader->specialization_ensure(specialization_map_entries);
auto &pipeline = shader->pipeline_get();
the auto keyword hides information for the developer and should IMO not be used.
I also don't think it is good to share a reference to the internal
unique_ptr<VKPipeline>
as now the caller is able to alter the VKPipeline instance without the owner to be aware of it.Would for this case still keep
VKPipeline& VKShader::pipeline_get()
as prototype. Just assert in pipeline_get when there is no instance.Still the question what is the reason to make this a unique ptr. I can see benefits of using unique ptr, But don't see what it has to do with the core of this patch (adding specialization constants)
Specialization constants are often used in drawing Eevee-next. Since there are multiple threads running there, its creation and termination must be unique.
@ -72,3 +82,2 @@
VKPipeline VKPipeline::create_graphics_pipeline(
const VKPushConstants::Layout &push_constants_layout)
void VKPipeline::create_graphics_pipeline(std::unique_ptr<VKPipeline> &pipeline,
I would have expected
std::unique_ptr<VKPipeline> create_graphics_pipeline(...)
as a prototype.create
is from the factory pattern https://en.wikipedia.org/wiki/Factory_method_pattern it seems you're using a https://en.wikipedia.org/wiki/Builder_patternThe factory pattern is better, but I have one question.
Once the specialization constants are successfully cached, a decision is made within the
create_compute_pipeline
function whether to regenerate theVkPipeline
.Also, if I don't need to regenerate it, what happens if I don't pass the
VKPipeline
inside the function?we can also consider caching information other than specialization constants.
. (Actually, I did it on another branch.)
In that case, we can decide whether to regenerate or not with this function, but wouldn't it be better to pass the
VKPipeline
?@ -602,0 +603,4 @@
{
VKShaderInterface *vk_interface = new VKShaderInterface();
vk_interface->init(info);
if (interface) {
Should never happen, perhaps add an assert
@ -686,4 +694,4 @@
}
if (result) {
interface = vk_interface;
unneeded code
@ -1001,1 +1008,3 @@
interface.init(info);
BLI_assert(interface);
VKShaderInterface &vk_interface = *(reinterpret_cast<VKShaderInterface *>(interface));
Would rather use an unwrap function like we do in other areas of the gpu module.
Should be a static cast, reinterpret means to tell the developer that you're not switching between compatible types (subclasses), but in this case you are.
that's right
To avoid any misunderstanding, I would like to explain the difference between OpenGL and Vulkan.
OpenGL's ShaderBind generates a program that reflects specialization constants. On the other hand, Vulkan propagates specialization constant changes to the pipeline rather than his ShaderProgram, so it doesn't do anything special during ShaderBind.
This is because most of the other information required for pipeline generation is not available. (VertexInput, blending, Depth_test, topology, etc.) So setting the flag to false at that point is sure to lead to confusion.
Only reseting a gpu dirty flag on one backend confuses developers when fixing issues on the other backends. Previously they were reset correctly, now they are not. Current approach is IMO confusing. Your initial vulkan specific flag would be more in line with this. Would suggest to propagate the gpu dirty flag to vulkan specific flag during bind.
In this case, there are many cases where the flags are not conveyed properly. I will investigate.
9da861af80
to0d3fd1bc7a
So far, there seems to be no problem.
Pull request closed