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

GL_EXT_mesh_shader/SPV_EXT_mesh_shader implementation #3014

Merged
merged 1 commit into from
Sep 2, 2022

Conversation

pmistryNV
Copy link
Contributor

Added following updates to GL_EXT_mesh_shader implementation:

  1. Added SPIRV and GLSL test cases

  2. Added checks to ensure NV and EXT mesh shader builtins cannot be used interchangeably.

  3. Updated the language name by removing the postfix "NV" to MeshShader and TaskShader.

  4. Added checks for grammar checking to comply with the spec.

  5. Added gl_NumWorkGroups builtin to Mesh shader

  6. Fixed data type of gl_PrimitiveLineIndicesEXT and gl_PrimitiveTriangleIndicesEXT

  7. Added new constants to the resources table

  8. Updates to handle new storage qualifier "taskPayloadSharedEXT"

  9. Updated test cases by replacing "taskEXT" with storage qualifier "taskPayloadSharedEXT"

Addressed Review comments

  1. Fixed instruction description used by glslang disassembly.
  2. Updated OpEmitMeshTasksEXT as per spec update
  3. Fixed implementation that errors out if there are more then one taskPayloadSharedEXT varjables.
  4. Fixed miscellaneous error logs and removed unwanted code.

SPIRV 1.6 related build failure fixes

  • Update SPIRV header to 1.6
  • Fix conflict wiht SPIRV 1.6 change, where localSizeId is used for execution mode for mesh/task shaders

Enable SPIRV generated for EXT_mesh_shader to be version 1.4

GL_EXT_mesh_shader: Add checks for atomic support and corresponding test cases

@BeastLe9enD
Copy link

BeastLe9enD commented Sep 1, 2022

I dont know if its the right place to notice, but I managed to produce an access violation compiling an mesh shader:
My shader code is the following:

#version 460

#extension GL_EXT_mesh_shader : require

layout(local_size_x = 1) in;
layout(triangles, max_vertices = 3, max_primitives = 1) out;

layout(location = 0) out vec3[] outColors;

void main() {
    SetMeshOutputsEXT(3, 1);

    gl_PrimitiveTriangleIndicesEXT[0] = uvec3(0, 1, 2);

    gl_MeshPerVertexEXT[0].gl_Position = vec4(-0.5, -0.5, 0.0, 1.0);
    gl_MeshPerVertexEXT[1].gl_Position = vec4(0.5, -0.5, 0.0, 1.0);
    gl_MeshPerVertexEXT[2].gl_Position = vec4(0.0, 0.5, 0.0, 1.0);

    outColors[0] = vec3(1.0, 0.0, 0.0);
    outColors[1] = vec3(0.0, 1.0, 0.0);
    outColors[2] = vec3(0.0, 0.0, 1.0);
}

The mistake in this shader code is that gl_MeshPerVertexEXT is wrong here, it should be gl_MeshVerticesEXT, but it should still not crash the compiler.

I tried to compile the shader with -V example.mesh.glsl -o example.mesh.spv --target-env spirv1.6 (because it complains without --target-env spirv1.6), but I get an access violation:

https://github.com/pmistryNV/glslang/blob/e9f59a226b8122b0ce796b3f547a4c1e03831017/glslang/MachineIndependent/ParseHelper.cpp#L508

As far as I debugged, the problem is that variable is nullptr, but it accesses variable which results in an access violation. Replacing if (language == EShLangMesh) with if (language == EShLangMesh && variable) solved the issue in my case.

@spnda
Copy link
Contributor

spnda commented Sep 1, 2022

  1. Updated the language name by removing the postfix "NV" to MeshShader and TaskShader.

I feel like this should have also be reflected in the C interface, which curiously did not get updated with KHR_ray_tracing:

typedef enum {
GLSLANG_STAGE_VERTEX,
GLSLANG_STAGE_TESSCONTROL,
GLSLANG_STAGE_TESSEVALUATION,
GLSLANG_STAGE_GEOMETRY,
GLSLANG_STAGE_FRAGMENT,
GLSLANG_STAGE_COMPUTE,
GLSLANG_STAGE_RAYGEN_NV,
GLSLANG_STAGE_INTERSECT_NV,
GLSLANG_STAGE_ANYHIT_NV,
GLSLANG_STAGE_CLOSESTHIT_NV,
GLSLANG_STAGE_MISS_NV,
GLSLANG_STAGE_CALLABLE_NV,
GLSLANG_STAGE_TASK_NV,
GLSLANG_STAGE_MESH_NV,
LAST_ELEMENT_MARKER(GLSLANG_STAGE_COUNT),
} glslang_stage_t; // would be better as stage, but this is ancient now

@pmistryNV
Copy link
Contributor Author

I dont know if its the right place to notice, but I managed to produce an access violation compiling an mesh shader: My shader code is the following:

#version 460

#extension GL_EXT_mesh_shader : require

layout(local_size_x = 1) in;
layout(triangles, max_vertices = 3, max_primitives = 1) out;

layout(location = 0) out vec3[] outColors;

void main() {
    SetMeshOutputsEXT(3, 1);

    gl_PrimitiveTriangleIndicesEXT[0] = uvec3(0, 1, 2);

    gl_MeshPerVertexEXT[0].gl_Position = vec4(-0.5, -0.5, 0.0, 1.0);
    gl_MeshPerVertexEXT[1].gl_Position = vec4(0.5, -0.5, 0.0, 1.0);
    gl_MeshPerVertexEXT[2].gl_Position = vec4(0.0, 0.5, 0.0, 1.0);

    outColors[0] = vec3(1.0, 0.0, 0.0);
    outColors[1] = vec3(0.0, 1.0, 0.0);
    outColors[2] = vec3(0.0, 0.0, 1.0);
}

The mistake in this shader code is that gl_MeshPerVertexEXT is wrong here, it should be gl_MeshVerticesEXT, but it should still not crash the compiler.

I tried to compile the shader with -V example.mesh.glsl -o example.mesh.spv --target-env spirv1.6 (because it complains without --target-env spirv1.6), but I get an access violation:

https://github.com/pmistryNV/glslang/blob/e9f59a226b8122b0ce796b3f547a4c1e03831017/glslang/MachineIndependent/ParseHelper.cpp#L508

As far as I debugged, the problem is that variable is nullptr, but it accesses variable which results in an access violation. Replacing if (language == EShLangMesh) with if (language == EShLangMesh && variable) solved the issue in my case.

Let me check it out

@pmistryNV
Copy link
Contributor Author

So what? Is there any reflection now on the failure of the 8 tests?
That's because the tools are not yet checked in. Tools PR KhronosGroup/SPIRV-Tools#4915 needs to be merged first.

@pmistryNV
Copy link
Contributor Author

  1. Updated the language name by removing the postfix "NV" to MeshShader and TaskShader.

I feel like this should have also be reflected in the C interface, which curiously did not get updated with KHR_ray_tracing:

typedef enum {
GLSLANG_STAGE_VERTEX,
GLSLANG_STAGE_TESSCONTROL,
GLSLANG_STAGE_TESSEVALUATION,
GLSLANG_STAGE_GEOMETRY,
GLSLANG_STAGE_FRAGMENT,
GLSLANG_STAGE_COMPUTE,
GLSLANG_STAGE_RAYGEN_NV,
GLSLANG_STAGE_INTERSECT_NV,
GLSLANG_STAGE_ANYHIT_NV,
GLSLANG_STAGE_CLOSESTHIT_NV,
GLSLANG_STAGE_MISS_NV,
GLSLANG_STAGE_CALLABLE_NV,
GLSLANG_STAGE_TASK_NV,
GLSLANG_STAGE_MESH_NV,
LAST_ELEMENT_MARKER(GLSLANG_STAGE_COUNT),
} glslang_stage_t; // would be better as stage, but this is ancient now

I updated the commit to address this comment

@spnda
Copy link
Contributor

spnda commented Sep 1, 2022

I updated the commit to address this comment

Thanks, I'm addressing the other enum values with #3015.

@pmistryNV
Copy link
Contributor Author

I updated the commit to address this comment

Thanks, I'm addressing the other enum values with #3015.

Are there users of the interface that also need to be updated? I checked shaderc and there is no dependency

@spnda
Copy link
Contributor

spnda commented Sep 1, 2022

I updated the commit to address this comment

Thanks, I'm addressing the other enum values with #3015.

Are there users of the interface that also need to be updated? I checked shaderc and there is no dependency

Well, I don't think the old values should be removed completely. In my PR I kept them as aliases to retain backwards compatibility. That way nobody needs to update. And I don't know any library that uses the C interface, so I can't really tell you.

@pmistryNV
Copy link
Contributor Author

I dont know if its the right place to notice, but I managed to produce an access violation compiling an mesh shader: My shader code is the following:

#version 460

#extension GL_EXT_mesh_shader : require

layout(local_size_x = 1) in;
layout(triangles, max_vertices = 3, max_primitives = 1) out;

layout(location = 0) out vec3[] outColors;

void main() {
    SetMeshOutputsEXT(3, 1);

    gl_PrimitiveTriangleIndicesEXT[0] = uvec3(0, 1, 2);

    gl_MeshPerVertexEXT[0].gl_Position = vec4(-0.5, -0.5, 0.0, 1.0);
    gl_MeshPerVertexEXT[1].gl_Position = vec4(0.5, -0.5, 0.0, 1.0);
    gl_MeshPerVertexEXT[2].gl_Position = vec4(0.0, 0.5, 0.0, 1.0);

    outColors[0] = vec3(1.0, 0.0, 0.0);
    outColors[1] = vec3(0.0, 1.0, 0.0);
    outColors[2] = vec3(0.0, 0.0, 1.0);
}

The mistake in this shader code is that gl_MeshPerVertexEXT is wrong here, it should be gl_MeshVerticesEXT, but it should still not crash the compiler.
I tried to compile the shader with -V example.mesh.glsl -o example.mesh.spv --target-env spirv1.6 (because it complains without --target-env spirv1.6), but I get an access violation:
https://github.com/pmistryNV/glslang/blob/e9f59a226b8122b0ce796b3f547a4c1e03831017/glslang/MachineIndependent/ParseHelper.cpp#L508
As far as I debugged, the problem is that variable is nullptr, but it accesses variable which results in an access violation. Replacing if (language == EShLangMesh) with if (language == EShLangMesh && variable) solved the issue in my case.

Let me check it out

I have pushed a fix for this as you have suggested. Thanks for pointing it out!!!

@pmistryNV
Copy link
Contributor Author

I updated the commit to address this comment

Thanks, I'm addressing the other enum values with #3015.

Are there users of the interface that also need to be updated? I checked shaderc and there is no dependency

Well, I don't think the old values should be removed completely. In my PR I kept them as aliases to retain backwards compatibility. That way nobody needs to update. And I don't know any library that uses the C interface, so I can't really tell you.

I updated the change to keep those old enum id's. Have a look and let me know if now it looks ok?

Added following updates to GL_EXT_mesh_shader implementation:

1. Added SPIRV and GLSL test cases
2. Added checks to ensure NV and EXT mesh shader builtins cannot be used interchangeably.
3. Updated the language name by removing the postfix "NV" to MeshShader and TaskShader.
4. Added checks for grammar checking to comply with the spec.

5. Added gl_NumWorkGroups builtin to Mesh shader
6. Fixed data type of gl_PrimitiveLineIndicesEXT and gl_PrimitiveTriangleIndicesEXT
7. Added new constants to the resources table
8. Updates to handle new storage qualifier "taskPayloadSharedEXT"
9. Updated test cases by replacing "taskEXT" with storage qualifier "taskPayloadSharedEXT"

Addressed  Review comments
1. Fixed instruction description used by glslang disassembly.
2. Updated OpEmitMeshTasksEXT as per spec update
3. Fixed implementation that errors out if there are more then one taskPayloadSharedEXT varjables.
4. Fixed miscellaneous error logs and removed unwanted code.

SPIRV 1.6 related build failure fixes
- Update SPIRV header to 1.6
- Fix conflict wiht SPIRV 1.6 change, where localSizeId is used for execution mode for mesh/task shaders

Enable SPIRV generated for EXT_mesh_shader to be version 1.4

GL_EXT_mesh_shader: Add checks for atomic support and corresponding test cases
@pmistryNV
Copy link
Contributor Author

pmistryNV commented Sep 2, 2022

@greg-lunarg @johnkslang kindly review and merge

@HansKristian-Work
Copy link

Looking to implement EXT_mesh_shader in SPIRV-Cross. Waiting for this to be merged :)

Copy link
Contributor

@greg-lunarg greg-lunarg left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the code!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants