Repository navigation
spirv-val: Add VUID 04681 check for specialization constant initializers - #6948
Tyagiquamar wants to merge 4 commits into
Conversation
Check that an OpVariable with a specialization constant initializer either has an OpDecorate with Constant or is in the Function storage class in Vulkan environments (VUID-StandaloneSpirv-Function-04681). Fixes KhronosGroup#6603.
| if (storage_class != spv::StorageClass::Function && | ||
| !_.HasDecoration(inst->id(), spv::Decoration::Constant)) { |
There was a problem hiding this comment.
The Constant decoration is only allowed with the Kernel capability so it won't ever be in the Vulkan environment.
Separately, I don't see how this matches the VU:
A type T that is an array sized with a specialization constant must neither be, nor be contained in, the type T2 of a variable V, unless either: a) T is equal to T2, b) V is declared in the Function, or Private Storage Class, c) V is a non-Block variable in the Workgroup Storage Class, or d) V is an interface variable with an additional level of arrayness, as described in interface matching, and T is the member type of the array type T2
There was a problem hiding this comment.
Thanks for the correction. Constant decoration is Kernel-only, so it is not used for Vulkan.
The check now enforces VUID-StandaloneSpirv-Function-04681 as written: a spec-constant-sized array type T must not be (or be contained in) a variable type T2 unless (a) T equals T2, (b) Function/Private, (c) non-Block Workgroup, or (d) an OpEntryPoint interface variable with the extra interface-matching arrayness where T is the member of T2 (aligned with validate_interfaces.cpp).
Latest commit: ec8b24e
PTAL.
|
Updated the validation in commit 21fd663 to accurately enforce VUID-StandaloneSpirv-Function-04681:
|
Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
21fd663 to
865d829
Compare
Match the StandaloneSpirv VU exceptions without Constant decoration, require OpEntryPoint interface listing for arrayed I/O, and cover StorageBuffer, PushConstant, TessellationControl, and Geometry output. Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
|
Ready for review. Addressed the Constant-decoration feedback and aligned the check with VUID-StandaloneSpirv-Function-04681 in ec8b24e. PTAL. |
| } | ||
|
|
||
| return SPV_SUCCESS; | ||
| // VUID-StandaloneSpirv-Function-04681 |
There was a problem hiding this comment.
This should be it's own helper function.
| return length && spvOpcodeIsSpecConstant(length->opcode()); | ||
| }; | ||
|
|
||
| if (!_.ContainsType(value_id, is_spec_constant_sized_array)) { |
There was a problem hiding this comment.
I think what you want is more like:
- Function or Private? return success
- Non-block workgroup? return success
- Let
check_idequalvalue_id. - If
check_idis a strippable interface array, letcheck_idbe its element type - If
check_idis an array, letcheck_idbe its element type - If
check_idcontains a spec-constant-sized array, return an error. - Return success.
| if (member_type && is_spec_constant_sized_array(member_type) && | ||
| !_.ContainsType(member_type->GetOperandAs<uint32_t>(1u), | ||
| is_spec_constant_sized_array)) { | ||
| const bool is_output = storage_class == spv::StorageClass::Output; |
There was a problem hiding this comment.
It feels like this is duplicated with code in validate_interfaces.cpp. That should be refactored in some way since it is non-trivial validation.
|
|
||
| TEST_F(ValidateMemory, VulkanSpecConstantSizedArrayInBlockWorkgroupBad) { | ||
| std::string spirv = R"( | ||
| OpCapability Shader |
There was a problem hiding this comment.
This should require the WorkgroupMemoryExplicitLayoutKHR capability (and extension).
Keep spec-constant-sized arrays out of variable types unless the variable is Function or Private, non-Block Workgroup, or the array is removed by one interface-matching strip plus one array level. Share that execution-model rule with interface location matching, and give the Block workgroup test the explicit-layout capability. Signed-off-by: Mohd Quamar Tyagi <104281681+Tyagiquamar@users.noreply.github.com>
Fixes #6603.
Root Cause
VUID-StandaloneSpirv-Function-04681requires that a typeTwhich is an array sized with a specialization constant must neither be, nor be contained in, the typeT2of a variableV, unless one of the listed exceptions applies (TequalsT2, Function/Private, non-Block Workgroup, or arrayed interface IO whereTis the member ofT2).The Constant decoration is Kernel-only and is not valid in Vulkan, so it must not be part of this check.
Changes
source/val/validation_state.cpp: map4681toVUID-StandaloneSpirv-Function-04681.source/val/validate_memory.cpp: inValidateVariableVulkanArray, enforce the array containment rule with the VU exceptions. Interface arrayness matchesvalidate_interfaces.cppand requires the variable to appear in anOpEntryPointinterface list.test/val/val_memory_test.cpp: positive and negative coverage for Uniform/StorageBuffer/PushConstant/Workgroup Block (bad), Function/Private/non-Block Workgroup/equal-type/Geometry and TessellationControl arrayed inputs (good), Vertex nested and Geometry output nested (bad).Verification
ec8b24e8.ValidateMemory.VulkanSpecConstant*runs were green on the previous commit; this push adds tests and tightens interface matching.