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.

Sign up for GitHub

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

Jump to bottom

spirv-val: Add missing SPV_EXT_mesh_shader validation #4919

Open
sjfricke opened this issue Sep 4, 2022 · 12 comments
Open

spirv-val: Add missing SPV_EXT_mesh_shader validation #4919

sjfricke opened this issue Sep 4, 2022 · 12 comments
Assignees
@pmistryNV
Labels
component:validation feature request

Comments

@sjfricke
Copy link
Contributor

sjfricke commented Sep 4, 2022

#4915 only added the ability for SPV_EXT_mesh_shader to be compatible, but there is no active new validation added yet.

@dgkoch is someone internally currently about to add this? Otherwise, I am happy to start adding the validation
@sjfricke sjfricke mentioned this issue Sep 5, 2022
Closed
@pmistryNV
Copy link
Contributor

pmistryNV commented Sep 6, 2022 •
edited
Loading

Can you be more specific what do you mean by active validation? Regarding OpEmitMeshTasksEXT I will be fixing it. It needs a spec update as well

@dgkoch
Copy link
Contributor

dgkoch commented Sep 6, 2022

@pmistryNV you only added "nop" support for the new builtin's etc. There needs to be proper validation added.

@sjfricke I'm not aware of anyone else working on it, so if you wanted to contribute - that would be great! We can use this issue to coordinate.

@sjfricke
Copy link
Contributor Author

sjfricke commented Sep 6, 2022 •
edited by spencer-lunarg
Loading

Here is the list I put together, I started on some of these, but wanted to not duplicate the effort, will clean them up later today and get some of them up

@sjfricke sjfricke mentioned this issue Sep 7, 2022
Merged
@spencer-lunarg spencer-lunarg mentioned this issue May 12, 2023
Closed
10 tasks
@godlikepanos
Copy link

It seems DXC is creating wrong output because it fails to follow Each OpEntryPoint with the MeshEXT Execution Model can have at most one global OpVariable of storage class TaskPayloadWorkgroupEXT. That creates all sort of problems and I wonder if it can be prioritized.

See microsoft/DirectXShaderCompiler#5981

@sudonatalie sudonatalie mentioned this issue Nov 21, 2023
Closed
@spencer-lunarg spencer-lunarg mentioned this issue Jan 12, 2024
Merged
@KarenGhavam-lunarG
Copy link

@sjfricke I'm not aware of anyone else working on it, so if you wanted to contribute - that would be great! We can use this issue to coordinate.

@dgkoch Unfortunately workloads are making it hard for LunarG to complete this missing validation. It would be best if Nvidia could complete it.

@pmistryNV
Copy link
Contributor

Let me take a look at it.

@dgkoch dgkoch assigned pmistryNV and unassigned spencer-lunarg Mar 18, 2024
@pmistryNV
Copy link
Contributor

@sjfricke regarding the test case
"OpSetMeshOutputsEXT must be called before any variable from Output storage class is written to" there is a corresponding runtime VUID VUID-RuntimeSpirv-MeshEXT-07118. Why do you think this should be determined statically?

@spencer-lunarg
Copy link
Contributor

VUID-RuntimeSpirv-MeshEXT-07118 would prevent you from going

// x == 2 from input
if (x > 1) {
    OpSetMeshOutputsEXT
}
if (x > 0) {
    OpSetMeshOutputsEXT
}

The wording in the spec now says

There must not be any control flow path to an output write that is not preceded by this instruction.

I read this as you could do it statically because you are not even allowed to have a potential if/switch/function with an output variable OpStore in it unless you have a OpSetMeshOutputsEXT before that block

If you feel this truly can't be done with static analysis, then it still needs a new Runtime VUID in the Vulkan spec then as it is different from 07118

@pmistryNV
Copy link
Contributor

VUID-RuntimeSpirv-MeshEXT-07118 would prevent you from going

// x == 2 from input
if (x > 1) {
    OpSetMeshOutputsEXT
}
if (x > 0) {
    OpSetMeshOutputsEXT
}

The wording in the spec now says

There must not be any control flow path to an output write that is not preceded by this instruction.

I read this as you could do it statically because you are not even allowed to have a potential if/switch/function with an output variable OpStore in it unless you have a OpSetMeshOutputsEXT before that block

If you feel this truly can't be done with static analysis, then it still needs a new Runtime VUID in the Vulkan spec then as it is different from 07118

Exactly because of non-uniform control flow this behavior cannot be determined statically. I will add a new VUID or probably update the current one to state this.

@spencer-lunarg
Copy link
Contributor

I will add a new VUID or probably update the current one to state this.

I would prefer a separate VU right next to 07118 as these are 2 different checks at runtime so rather have 2 VUs

@pmistryNV
Copy link
Contributor

Still working on the changes. I also encountered another validation missing "VUID-PrimitiveId-PrimitiveId-07040". So I am looking into all the validation that needs to be covered for the extension.

This was referenced Apr 3, 2024
Closed
Open
@pmistryNV
Copy link
Contributor

Created PR #5640.
Now looking into https://gitlab.khronos.org/vulkan/vulkan/-/issues/3296

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pmistryNV pmistryNV

Labels
component:validation feature request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@godlikepanos @dgkoch @sjfricke @KarenGhavam-lunarG @s-perron @pmistryNV @spencer-lunarg and others