Vulkan: Add: Specialization constants #116952

Closed
Kazashi Yoshioka wants to merge 14 commits from (deleted):vk_specialization_constants into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Contributor

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.

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.
Kazashi Yoshioka added 1 commit 2024-01-09 17:46:46 +01:00
83b5b89c2e Vulkan: Add: Specialization constants
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.
Kazashi Yoshioka added 1 commit 2024-01-09 17:50:00 +01:00
Kazashi Yoshioka requested review from Jeroen Bakker 2024-01-09 17:50:58 +01:00
Jeroen Bakker added this to the 4.1 milestone 2024-01-10 14:55:18 +01:00
Jeroen Bakker added the
Interest
Vulkan
label 2024-01-10 14:55:26 +01:00
Jeroen Bakker added this to the EEVEE & Viewport project 2024-01-10 14:55:31 +01:00
Jeroen Bakker reviewed 2024-01-11 15:40:16 +01:00
Jeroen Bakker left a comment
Member

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.

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.
Kazashi Yoshioka added 1 commit 2024-01-15 13:18:57 +01:00
Kazashi Yoshioka added 3 commits 2024-01-15 14:47:49 +01:00
08ce9edf10 Vulkan: Fix: Specialization Constants
Building specialization constants using `Shader::Bind` means owning the `VkSpecializationInfo`.
This also increases ownership of `VkSpecializationMapEntry`.
So I think it's a good idea to propagate the dirty flag without doing that.
As you said, the Vector cache is no longer needed.
69a3004891 Vulkan: Fix: Shader Init
By adding `Shader::Init`, `VKShaderInterface` will be initialized correctly.
Author
Contributor

Added support for changes to OpenGL. Thank you for your review.

Added support for changes to OpenGL. Thank you for your review.
Jeroen Bakker reviewed 2024-01-18 12:21:46 +01:00
@ -38,3 +39,3 @@
}
VKPipeline VKPipeline::create_compute_pipeline(
VKPipeline* VKPipeline::create_compute_pipeline(
Member

Better use std::unique_ptr<VKPipeline> as return type. Unsure if this change is actually needed.

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));
Member

If this is needed and .fl cannot be used, we should comment why,

If this is needed and `.fl` cannot be used, we should comment why,
vnapdv marked this conversation as resolved
@ -1407,0 +1429,4 @@
VkSpecializationInfo null = {};
return null;
}
static Vector<VkSpecializationMapEntry> entries;
Member

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() for mapEntryCount

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()` for `mapEntryCount`
vnapdv marked this conversation as resolved
@ -1407,0 +1443,4 @@
/* const void *pData */ constants.values.data()};
};
const VkSpecializationInfo VKShader::specialzation_ensure()
Member

spelling mistake

spelling mistake
vnapdv marked this conversation as resolved
@ -33,2 +33,2 @@
VKPipeline pipeline_;
std::unique_ptr<VKPipeline> pipeline_;
bool vk_specialtization_dirty_ = false;
Member

spelling

spelling
vnapdv marked this conversation as resolved
Kazashi Yoshioka added 1 commit 2024-01-18 18:05:57 +01:00
Kazashi Yoshioka added 1 commit 2024-01-18 23:39:43 +01:00
5c403b966a Vulkan: Refactor: Specialization Constants
VkSpecializationInfo and VkSpecializationMapEntry should only be used immediately before generation,
as they can cause confusion during multi-threaded execution. Unique_ptr is also used for the same reason.
The question is, can we do anything with Shader-Bind?
At this point, it is impossible to generate a Pipeline, so if we do not make it a through path, it will be completely broken.
I tried doing a simple Shading of EEvEE_Next on another branch.
Setting the constants.is_dirty flag to false in shader-bind makes pipeline generation probabilistic.
Author
Contributor

There have been a few changes.
Please review again.

There have been a few changes. Please review again.
Jeroen Bakker requested changes 2024-01-19 08:33:17 +01:00
@ -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;
Member

OpenGL requires this. changing this will break the opengl implementation.

sequenceDiagram
    participant GPU_shader_bind
    participant VKShader
    participant VKPipeline

    activate GPU_shader_bind
    GPU_shader_bind --> VKShader: bind()
    activate VKShader
    VKShader --> VKPipeline: update_specialization_constants()

    activate VKPipeline
    VKPipeline --> VKPipeline: is_dirty = true
    deactivate VKPipeline
    
    deactivate VKShader

    GPU_shader_bind --> VKShader: constants.is_dirty = false
    deactivate GPU_shader_bind

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.

OpenGL requires this. changing this will break the opengl implementation. ```mermaid sequenceDiagram participant GPU_shader_bind participant VKShader participant VKPipeline activate GPU_shader_bind GPU_shader_bind --> VKShader: bind() activate VKShader VKShader --> VKPipeline: update_specialization_constants() activate VKPipeline VKPipeline --> VKPipeline: is_dirty = true deactivate VKPipeline deactivate VKShader GPU_shader_bind --> VKShader: constants.is_dirty = false deactivate GPU_shader_bind ``` 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.
Author
Contributor

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 the is_dirty flag is True, so as long as it is set to false there, the system logic remains unchanged.

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 the `is_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();
Member

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)

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)
Author
Contributor

Specialization constants are often used in drawing Eevee-next. Since there are multiple threads running there, its creation and termination must be unique.

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,
Member

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_pattern

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_pattern
Author
Contributor

The 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 the VkPipeline.
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?

The 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 the `VkPipeline`. 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) {
Member

Should never happen, perhaps add an assert

Should never happen, perhaps add an assert
vnapdv marked this conversation as resolved
@ -686,4 +694,4 @@
}
if (result) {
interface = vk_interface;
Member

unneeded code

unneeded code
vnapdv marked this conversation as resolved
@ -1001,1 +1008,3 @@
interface.init(info);
BLI_assert(interface);
VKShaderInterface &vk_interface = *(reinterpret_cast<VKShaderInterface *>(interface));
Member

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.

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.
Author
Contributor

that's right

that's right
vnapdv marked this conversation as resolved
Kazashi Yoshioka added 1 commit 2024-01-19 14:34:55 +01:00
Author
Contributor

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.

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.
Kazashi Yoshioka added 1 commit 2024-01-19 15:46:55 +01:00
Kazashi Yoshioka added 1 commit 2024-01-19 15:50:51 +01:00
Kazashi Yoshioka requested review from Jeroen Bakker 2024-01-19 15:52:42 +01:00
Jeroen Bakker reviewed 2024-01-22 10:58:50 +01:00
Jeroen Bakker left a comment
Member

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.

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.
Author
Contributor

In this case, there are many cases where the flags are not conveyed properly. I will investigate.

In this case, there are many cases where the flags are not conveyed properly. I will investigate.
Kazashi Yoshioka added 1 commit 2024-01-25 19:30:30 +01:00
Kazashi Yoshioka force-pushed vk_specialization_constants from 9da861af80 to 0d3fd1bc7a 2024-01-29 12:57:38 +01:00 Compare
Kazashi Yoshioka added 1 commit 2024-01-29 12:59:19 +01:00
Kazashi Yoshioka added 1 commit 2024-01-29 13:02:23 +01:00
Author
Contributor

So far, there seems to be no problem.

So far, there seems to be no problem.
Kazashi Yoshioka closed this pull request 2024-02-16 11:23:08 +01:00

Pull request closed

Sign in to join this conversation.
No reviewers
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 Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#116952
No description provided.