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

Add EXT_mesh_shader validation support #5640

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pmistryNV
Copy link
Contributor

@pmistryNV pmistryNV commented Apr 10, 2024

  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

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
Copy link
Contributor Author

@spencer-lunarg kindly review.

Copy link
Contributor

@spencer-lunarg spencer-lunarg left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

did you forget to remove this?

}

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

Choose a reason for hiding this comment

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

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

// 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() ||
Copy link
Contributor

Choose a reason for hiding this comment

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

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() ||

Comment on lines +147 to +148
if (target->opcode() != spv::Op::OpVariable) {
return fail(0) << "must be a memory object declaration";
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Comment on lines +370 to +377
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";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

uint32_t GetMeshEntryPoint() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

sudonatalie added a commit to sudonatalie/DirectXShaderCompiler that referenced this pull request Apr 12, 2024
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
sudonatalie added a commit to microsoft/DirectXShaderCompiler that referenced this pull request Apr 17, 2024
)

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
Copy link
Contributor

@pmistryNV any update on the changes for this PR?

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.

4 participants