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

Fix the CullPrimitiveEXT VUID's #2475

Merged

Conversation

pmistryNV
Copy link
Contributor

  • Remove VUID 07037 that wrongly was needing the builtin to be of type array
  • Updated the VUID 7036 to make the type a boolean value

Fixes bug https://gitlab.khronos.org/vulkan/vulkan/-/issues/3296

@CLAassistant
Copy link

CLAassistant commented Dec 24, 2024

CLA assistant check
All committers have signed the CLA.

@pmistryNV pmistryNV force-pushed the vkissue_3296_cullprimitiveEXT_VUID branch from 0ce6b9b to 370033f Compare January 7, 2025 17:47
@pmistryNV pmistryNV force-pushed the vkissue_3296_cullprimitiveEXT_VUID branch from 370033f to 35c1e28 Compare January 7, 2025 17:56
Copy link
Contributor

@dgkoch dgkoch left a comment

Choose a reason for hiding this comment

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

LGTM

@HansKristian-Work
Copy link
Contributor

I don't think this is correct.

When CullPrimitiveEXT is part of a Block, it must be a plain bool, but if it's declared as a standalone OpVariable, i.e. not in a Block, it must be an array of booleans, so I think this VUID needs to cover both cases.

Copy link
Contributor

@HansKristian-Work HansKristian-Work left a comment

Choose a reason for hiding this comment

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

Not correct I think as stated above.

@pmistryNV
Copy link
Contributor Author

Not correct I think as stated above.

GLSL spec has folowing definition

      // write only access
      perprimitiveEXT out gl_MeshPerPrimitiveEXT {
        int  gl_PrimitiveID;
        int  gl_Layer;
        int  gl_ViewportIndex;
        bool gl_CullPrimitiveEXT;
        int  gl_PrimitiveShadingRateEXT;
      } gl_MeshPrimitivesEXT[];

Then the array validation rule will applly to all the other builtins like PrimitiveId , Layer etc? Because currently none of the other builtins have such a validation rule

@pmistryNV
Copy link
Contributor Author

Not correct I think as stated above.

GLSL spec has folowing definition

      // write only access
      perprimitiveEXT out gl_MeshPerPrimitiveEXT {
        int  gl_PrimitiveID;
        int  gl_Layer;
        int  gl_ViewportIndex;
        bool gl_CullPrimitiveEXT;
        int  gl_PrimitiveShadingRateEXT;
      } gl_MeshPrimitivesEXT[];

Then the array validation rule will applly to all the other builtins like PrimitiveId , Layer etc? Because currently none of the other builtins have such a validation rule

We will then probably need new VUID's for each of the above builtins. Kindly review these three new VUID's for CullPrimitiveEXT, i can replicate it for all the other builtins:

  • CullPrimitiveEXT can be member of a block or a variable
  • If CullPrimitiveEXT is a variable it must be declared as an array of boolean values
  • If CullPrimitiveEXT is member of a block it must be declared as a boolean value

@pmistryNV pmistryNV force-pushed the vkissue_3296_cullprimitiveEXT_VUID branch from 35c1e28 to e6e4c04 Compare January 10, 2025 21:15
@pmistryNV
Copy link
Contributor Author

  • If CullPrimitiveEXT is a variable it must be declared as an array of boolean values

@HansKristian-Work can you review the new VUID's. Based on how they look, I will add VUID's for other builtins defined in gl_MeshPerPrimitiveEXT

@pmistryNV pmistryNV force-pushed the vkissue_3296_cullprimitiveEXT_VUID branch from e6e4c04 to 95ed035 Compare January 10, 2025 21:58
@pmistryNV
Copy link
Contributor Author

@HansKristian-Work i have updated VUID's for all the relevant builtins

Copy link
Contributor

@HansKristian-Work HansKristian-Work left a comment

Choose a reason for hiding this comment

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

Some micronits but should be fine.

@pmistryNV pmistryNV force-pushed the vkissue_3296_cullprimitiveEXT_VUID branch from 8cd14f1 to 2e0f3dd Compare February 12, 2025 17:27
@pmistryNV
Copy link
Contributor Author

@dgkoch @HansKristian-Work kindly approve the changes if everything looks fine

pmistryNV and others added 3 commits February 12, 2025 09:32
be used as part of a structure or as a array of boolean. Corresponding
VUID changes are also needed for PrimitiveID, Layer, ViewPortIndex,
PrimitiveShadingRateEXT
Update the validation rules. Your comments look good. I will add it for rest of the builtins

Co-authored-by: Hans-Kristian Arntzen <[email protected]>
…storage qualifier

Update the VUID's for builtins Layer, PrimitiveId, PrimitiveShadingRateKHR, ViewPortIndex similar to CullPrimitiveEXT as they can be part of a block or can be individual arrays for MeshEXT execution model
@pmistryNV pmistryNV force-pushed the vkissue_3296_cullprimitiveEXT_VUID branch from 2e0f3dd to 8f41092 Compare February 12, 2025 17:33
Copy link
Contributor

@HansKristian-Work HansKristian-Work left a comment

Choose a reason for hiding this comment

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

It'll still need to be signed off in the working group though.

@dgkoch dgkoch added this to the Signed-off to Merge milestone Feb 20, 2025
@dgkoch
Copy link
Contributor

dgkoch commented Feb 20, 2025

LGTM again. Signing off as requested on the maint call.

@oddhack
Copy link
Contributor

oddhack commented Feb 21, 2025

The CI failure is not this branch's doing so merging this.

@oddhack oddhack merged commit 54195f7 into KhronosGroup:main Feb 21, 2025
9 of 10 checks passed
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.

5 participants