Add EXT_mesh_shader validation support #5640

pmistryNV · 2024-04-10T20:39:12Z

Each OpEntryPoint with the MeshEXT Execution Model can have at most one global OpVariable of storage class TaskPayloadWorkgroupEXT.
PerPrimitiveEXT only be used on a memory object declaration or a member of a structure type
PerPrimitiveEXT only Input in Fragment and Output in MeshEXT
Added Mesh vulkan validation support for following rules: VUID-Layer-Layer-07039 VUID-PrimitiveId-PrimitiveId-07040,VUID-PrimitivePointIndicesEXT-PrimitivePointIndicesEXT-07042, VUID-PrimitiveLineIndicesEXT-PrimitiveLineIndicesEXT-07048, VUID-PrimitiveTriangleIndicesEXT-PrimitiveTriangleIndicesEXT-07054, VUID-ViewportIndex-ViewportIndex-07060 VUID-StandaloneSpirv-ExecutionModel-07330 VUID-StandaloneSpirv-ExecutionModel-07331

For bug #4919

1. Each OpEntryPoint with the MeshEXT Execution Model can have at most one global OpVariable of storage class TaskPayloadWorkgroupEXT. 2. PerPrimitiveEXT only be used on a memory object declaration or a member of a structure type 3. PerPrimitiveEXT only Input in Fragment and Output in MeshEXT 4. Added Mesh vulkan validation support for following rules: VUID-Layer-Layer-07039 VUID-PrimitiveId-PrimitiveId-07040,VUID-PrimitivePointIndicesEXT-PrimitivePointIndicesEXT-07042, VUID-PrimitiveLineIndicesEXT-PrimitiveLineIndicesEXT-07048, VUID-PrimitiveTriangleIndicesEXT-PrimitiveTriangleIndicesEXT-07054, VUID-ViewportIndex-ViewportIndex-07060 VUID-StandaloneSpirv-ExecutionModel-07330 VUID-StandaloneSpirv-ExecutionModel-07331

pmistryNV · 2024-04-10T22:46:47Z

@spencer-lunarg kindly review.

spencer-lunarg

I'll try to do a deep review soon, but some small nits from a quick scan

spencer-lunarg · 2024-04-11T00:41:26Z

source/val/validate_builtins.cpp

+                  "Builtin PrimitiveId within the MeshEXT Execution Model must "
+                  "also be decorated with the PerPrimitiveEXT decoration. ";
+      }
+#if 0


did you forget to remove this?

spencer-lunarg · 2024-04-11T00:43:47Z

source/val/validate_mesh_shading.cpp

+          }
+
+          if (meshInterfaceVar && !storage_output) {
+            std::string vkerror = (spvIsVulkanEnv(_.context()->target_env))


VkErrorID already will return "" if its not a Vulkan Env, so you don't need to do that here

I intend to print the error irrespective of whether the SPIRV is used for vulkan environment or not. That is the reason.

yes, if you go just go _.VkErrorID(4336) it will already return the correct message for you regardless of the environment

spencer-lunarg · 2024-04-11T00:46:09Z

source/val/validate_builtins.cpp

+        // Every entry point from which this function is called needs to have
+        // Execution Mode DepthReplacing.
+        const auto* models = _.GetExecutionModels(entry_point);
+        if (models->find(spv::ExecutionModel::MeshEXT ) != models->end() ||


the CI bot has not kicked off yet, but it will for sure fail for not running clang-format, so I would suggest running it on this PR

Suggested change

if (models->find(spv::ExecutionModel::MeshEXT ) != models->end() ||

if (models->find(spv::ExecutionModel::MeshEXT) != models->end() ||

alan-baker · 2024-04-11T15:16:43Z

source/val/validate_annotation.cpp

+      if (target->opcode() != spv::Op::OpVariable) {
+        return fail(0) << "must be a memory object declaration";


Memory object declarations include OpFunctionParameter (and are handled in the next block of decorations). The spec matches that, is this a bug here or in the extension?

alan-baker · 2024-04-11T15:18:14Z

source/val/validate_annotation.cpp

+  const auto target_id = inst->GetOperandAs<uint32_t>(0);
+  const auto target = _.FindDef(target_id);
+  if (decoration == spv::Decoration::PerPrimitiveNV && 
+    target->opcode() != spv::Op::OpTypeStruct) {
+      return _.diag(SPV_ERROR_INVALID_ID, inst)
+             << _.SpvDecorationString(decoration)
+             << " must be a memory object declaration";
+  }


This seems like it would be caught above already at line 346.

alan-baker · 2024-04-11T15:21:10Z

source/val/validate_builtins.cpp

@@ -667,6 +667,36 @@ class BuiltInsValidator {
  // instruction.
  void Update(const Instruction& inst);

+  uint32_t GetMeshEntryPoint() {


Can you only have a single mesh entry point in a module? I'm confused why this is a single entry point.

alan-baker · 2024-04-11T15:25:51Z

source/val/validate_decorations.cpp

@@ -779,6 +780,19 @@ spv_result_t CheckDecorationsOfEntryPoints(ValidationState_t& vstate) {
        }
        const spv::StorageClass storage_class =
            var_instr->GetOperandAs<spv::StorageClass>(2);
+        if (vstate.version() >= SPV_SPIRV_VERSION_WORD(1, 5)) {


Why is this version gated on 1.5? SPV_EXT_mesh_shader requires SPIR-V 1.4. What is intended to be checked here?

It should be 1.4, I am not sure why i added 1.5.
The intention here is to satisfy the requirement in SPV_EXT_mesh_shader spec
"There can be at most one of type OpVariable with storage class TaskPayloadWorkgroupEXT associated with an OpEntryPoint. This OpVariable should be a global OpVariable."

Basically we cannot have more then one taskPayloadSharedEXT type global variable associated with one entrypoint

The existing logic from VK_NV_mesh_shader was incorrectly adapted for the VK_EXT_mesh_shader implementation when it comes to the handling of mesh payloads as in/out variables. Because TaskPayloadWorkgroupEXT must be applied to a single global OpVariable for each Task/Mesh shader, the struct should not be flattened. Further, as far as I can tell, Location assignment is not necessary for these input and output variables, so the usual reason for flattening structs does not apply. This change now removes the inner struct member global variables and ensures the parent payload is decorated with TaskPayloadWorkgroupEXT. Note that for amplification/task shaders, the payload variable is created with the groupshared decl, and then it's storage class needs to be updated when that variable is used as a parameter to the DispatchMesh call, as described in: https://docs.vulkan.org/spec/latest/proposals/proposals/VK_EXT_mesh_shader.html#_hlsl_changes Tested with updated spirv-val from: KhronosGroup/SPIRV-Tools#5640 Fixes microsoft#5981

) The existing logic from `VK_NV_mesh_shader` was incorrectly adapted for the `VK_EXT_mesh_shader` implementation when it comes to the handling of payloads as in/out variables. Because `TaskPayloadWorkgroupEXT` must be applied to a single global `OpVariable` for each task/mesh shader, the struct should not be flattened. Further, Location assignment is not necessary for these input and output variables, so the usual reason for flattening structs does not apply. This change now removes the inner struct member global variables and ensures the parent payload is decorated with `TaskPayloadWorkgroupEXT`. Note that for amplification/task shaders, the payload variable is created with the `groupshared` decl, and then its storage class needs to be updated when that variable is used as a parameter to the `DispatchMesh` call, as described in: https://docs.vulkan.org/spec/latest/proposals/proposals/VK_EXT_mesh_shader.html#_hlsl_changes Tested with new validation checks from: KhronosGroup/SPIRV-Tools#5640 Fixes #5981

spencer-lunarg · 2024-06-20T16:40:49Z

@pmistryNV any update on the changes for this PR?

danginsburg · 2024-11-27T20:52:43Z

Any updates on when this might be completed? I'm adding VK_EXT_mesh_shader support to our engine and would like to make sure validation is hooked up properly.

KarenGhavam-lunarG · 2024-11-27T20:59:18Z

Any updates on when this might be completed? I'm adding VK_EXT_mesh_shader support to our engine and would like to make sure validation is hooked up properly.

It would be good to know when this will be completed. I have had people from the ecosystem complain about lack of validation support for this extension

pmistryNV mentioned this pull request Apr 10, 2024

spirv-val: Add missing SPV_EXT_mesh_shader validation #4919

Open

pmistryNV force-pushed the mesh_shader_Validation branch from c00d507 to fdd61c9 Compare April 10, 2024 20:50

pmistryNV force-pushed the mesh_shader_Validation branch from fdd61c9 to 7e583d0 Compare April 10, 2024 22:24

spencer-lunarg requested a review from alan-baker April 11, 2024 00:40

spencer-lunarg reviewed Apr 11, 2024

View reviewed changes

alan-baker added the kokoro:run label Apr 11, 2024

kokoro-team removed the kokoro:run label Apr 11, 2024

alan-baker reviewed Apr 11, 2024

View reviewed changes

sudonatalie mentioned this pull request Apr 12, 2024

[SPIR-V] Fix mesh payload global variable for VK_EXT_mesh_shaders microsoft/DirectXShaderCompiler#6526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EXT_mesh_shader validation support #5640

Add EXT_mesh_shader validation support #5640

pmistryNV commented Apr 10, 2024 •

edited

Loading

pmistryNV commented Apr 10, 2024

spencer-lunarg left a comment

spencer-lunarg Apr 11, 2024

spencer-lunarg Apr 11, 2024

pmistryNV Dec 20, 2024

spencer-lunarg Dec 22, 2024

spencer-lunarg Apr 11, 2024

alan-baker Apr 11, 2024

alan-baker Apr 11, 2024

alan-baker Apr 11, 2024

alan-baker Apr 11, 2024

pmistryNV Dec 20, 2024

spencer-lunarg commented Jun 20, 2024

danginsburg commented Nov 27, 2024

KarenGhavam-lunarG commented Nov 27, 2024

	if (models->find(spv::ExecutionModel::MeshEXT ) != models->end() \|\|
	if (models->find(spv::ExecutionModel::MeshEXT) != models->end() \|\|

		if (target->opcode() != spv::Op::OpVariable) {
		return fail(0) << "must be a memory object declaration";

Add EXT_mesh_shader validation support #5640

Are you sure you want to change the base?

Add EXT_mesh_shader validation support #5640

Conversation

pmistryNV commented Apr 10, 2024 • edited Loading

pmistryNV commented Apr 10, 2024

spencer-lunarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencer-lunarg commented Jun 20, 2024

danginsburg commented Nov 27, 2024

KarenGhavam-lunarG commented Nov 27, 2024

pmistryNV commented Apr 10, 2024 •

edited

Loading