Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added required_subgroup_size to PipelineShaderStageCreateInfo #2235

Merged
merged 16 commits into from
Aug 18, 2023

Conversation

charles-r-earp
Copy link
Contributor

Compute and Graphics pipelines can be created with a required subgroup size.

https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_EXT_subgroup_size_control.html

To do

  • Validation / validation errors.
  • Tests

Limitations

For compute, can't validate max_compute_workgroup_subgroups without knowing the workgroup size, which is not currently reflected.

This interacts with additional VK_EXT_subgroup_size_control flags for requiring full subgroups and allowing varying subgroups, but since those aren't currently implemented they were neglected. If implemented, they add additional constraints to validate.

@Rua
Copy link
Contributor

Rua commented Jun 24, 2023

#2234 will make some changes to how PipelineShaderStageCreateInfo is validated, so it may be better to wait until that is merged.

@@ -191,12 +203,14 @@ impl ComputePipeline {
let specialization_info_vk;
let specialization_map_entries_vk: Vec<_>;
let mut specialization_data_vk: Vec<u8>;
let required_subgroup_size_create_info;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the variables holding data to be passed to Vulkan end in _vk. Could you include that as well please?

vulkano/src/shader/mod.rs Outdated Show resolved Hide resolved
vulkano/src/shader/mod.rs Outdated Show resolved Hide resolved
@Rua
Copy link
Contributor

Rua commented Jun 26, 2023

#2234 has been merged now.

@Rua
Copy link
Contributor

Rua commented Jul 19, 2023

@charles-r-earp are you still interested in getting this merged?

@charles-r-earp
Copy link
Contributor Author

@charles-r-earp are you still interested in getting this merged?

Yes

@charles-r-earp charles-r-earp marked this pull request as ready for review July 27, 2023 21:23
@Rua
Copy link
Contributor

Rua commented Jul 28, 2023

A few points:

  • You forgot to include the format! macro when creating most of the error messages. That means that for example {subgroup_size} will appear literally in the message, not the value of the variable.
  • Please put backticks around the variable and property names in the problem field (but not the context field).
  • In a few places you wrote an error message instead of a VUID.
  • VUID-VkPipelineShaderStageCreateInfo-pNext-02756 still needs to be validated.
  • Can you fix the review points I mentioned earlier?

vulkano/src/pipeline/mod.rs Outdated Show resolved Hide resolved
vulkano/src/pipeline/mod.rs Outdated Show resolved Hide resolved
@charles-r-earp
Copy link
Contributor Author

  • VUID-VkPipelineShaderStageCreateInfo-pNext-02756 still needs to be validated.

This is validated:

if let Some(workgroup_size) = workgroup_size {
    let max_compute_workgroup_subgroups = properties
        .max_compute_workgroup_subgroups
        .unwrap_or_default();
    if max_compute_workgroup_subgroups * subgroup_size < workgroup_size {
        return Err(Box::new(ValidationError {
            context: "required_subgroup_size".into(),
            problem: format!("`subgroup_size` {subgroup_size} creates more than {max_compute_workgroup_subgroups} subgroups").into(),
            vuids: &["VUID-VkPipelineShaderStageCreateInfo-pNext-02756"],
            ..Default::default()
        }));
    }
}

However, workgroup_size is not available because it's not reflected from the shader, so it's currently None.

@Rua
Copy link
Contributor

Rua commented Jul 28, 2023

Can you add the necessary reflection then?

if !properties
.required_subgroup_size_stages
.unwrap_or_default()
.contains(stages)
Copy link
Contributor

@Rua Rua Jul 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use contains_enum(stage) here and then you don't need to do the conversion above.

vulkano/src/shader/mod.rs Outdated Show resolved Hide resolved
@charles-r-earp charles-r-earp requested a review from Rua August 7, 2023 19:14
@Rua
Copy link
Contributor

Rua commented Aug 8, 2023

It looks good so far, but I noticed that the required_subgroup_size test is causing an overflow when I run it:

---- pipeline::compute::tests::required_subgroup_size stdout ----
thread 'pipeline::compute::tests::required_subgroup_size' panicked at 'attempt to multiply with overflow', vulkano/src/pipeline/mod.rs:582:12

It looks like max_compute_workgroup_subgroups * subgroup_size can be too big to fit into an u32. Perhaps the expression can be rearranged so that an overflow can't happen.

@charles-r-earp
Copy link
Contributor Author

It looks like max_compute_workgroup_subgroups * subgroup_size can be too big to fit into an u32. Perhaps the expression can be rearranged so that an overflow can't happen.

While subgroup_size is up to 128, max_compute_subgroups can be u32::MAX so I decided to use checked_mul for this. But I also realized that this was a problem for local_size too because while the product of [x, y, z] must be a u32, each dimension can be u32::MAX as well, so just casting to u64 seems simpler.

@Rua
Copy link
Contributor

Rua commented Aug 9, 2023

But I also realized that this was a problem for local_size too because while the product of [x, y, z] must be a u32, each dimension can be u32::MAX as well, so just casting to u64 seems simpler.

u32::MAX * u32::MAX * u32::MAX is still bigger than u64::MAX though!

@charles-r-earp
Copy link
Contributor Author

u32::MAX * u32::MAX * u32::MAX is still bigger than u64::MAX though!

Nice catch!

vulkano/src/pipeline/mod.rs Outdated Show resolved Hide resolved
@Rua
Copy link
Contributor

Rua commented Aug 18, 2023

Looks good now, thank you!

@Rua Rua merged commit ee4e308 into vulkano-rs:master Aug 18, 2023
2 of 3 checks passed
Rua added a commit that referenced this pull request Aug 18, 2023
Rua added a commit that referenced this pull request Aug 18, 2023
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
…o-rs#2235)

* Added required_subgroup_size to PipelineShaderStageCreateInfo

* Added validation errors.

* Fixed error msgs / vuids.

* ComputeShaderExecution for validating local_size.

* WorkgroupSizeId reflection.

* contains_enum

* Reworked ComputeShaderExecution.

* panic msgs.

* workgroup size validation

* unused import

* fixed test deprecated fn

* catch workgroup size overflow

* EntryPointInfo::local_size docs

* comments

* typo + error msg
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants