[val] Explicit layout refactor#6792
Conversation
* Combine layout checks into a single pass to share logic * checks both required explicit layouts and invalid layouts * remove checks from validate_decorations.cpp * Introduce enums for layout modes and requirements * Centralize which instructions are examined for layouts * Add options to specify descriptor sizes to the validator (buffer, sampler, image, and tensor)
* Fix handling of OpBufferPointerEXT * Add tests for missing coverage
| auto sc = storage_class_ptr->GetOperandAs<spv::StorageClass>(1u); | ||
| if (sc != spv::StorageClass::StorageBuffer && | ||
| sc != spv::StorageClass::Uniform) { | ||
| return _.diag(SPV_ERROR_INVALID_ID, inst) | ||
| << "OpBufferPointerEXT Result Type must be a pointer type " | ||
| << "with a Storage Class of Uniform or StorageBuffer."; | ||
| } |
There was a problem hiding this comment.
This was previously in CheckDecorationsOfBuffers, but makes more sense here.
|
|
||
| if (inst->opcode() == spv::Op::OpConstantNull) { | ||
| *val = 0; | ||
| } else if (inst->opcode() == spv::Op::OpConstantSizeOfEXT) { |
There was a problem hiding this comment.
These allow validation to process OpConstantSizeOf without freezing the constants to some value.
| return SPV_SUCCESS; | ||
| } | ||
|
|
||
| spv_result_t CheckDecorationsOfBuffers(ValidationState_t& vstate) { |
There was a problem hiding this comment.
I left the block checks since I don't entirely count them as explicit layout.
|
I should mention as well, I tested this against vk-gl-cts main branch and the only validation problems looked real and related workgroup memory explicit layout. See vk-gl-cts tracker issue 6736. |
| @@ -1073,130 +1073,6 @@ TEST_F(ValidateDecorations, BlockDecoratingIntBad) { | |||
| EXPECT_THAT(getDiagnosticString(), HasSubstr("must be a structure type")); | |||
| } | |||
|
|
|||
| TEST_F(ValidateDecorations, BlockMissingOffsetBad) { | |||
There was a problem hiding this comment.
All these tests were ported straight into the new file (modulo some message updates).
| EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0)); | ||
| } | ||
|
|
||
| TEST_F(ValidateExplicitLayout, BufferPointerEXTMissingOffsetBad) { |
There was a problem hiding this comment.
These are new until the untyped layout tests.
spencer-lunarg
left a comment
spencer-lunarg
left a comment
There was a problem hiding this comment.
I tested with VVL and CTS, it looks good will likely need some VUID churn... but honestly that is a followup level PR
overall this REALLY helps with untyped pointers to enforce people from setting offset/arrayStride to crazy non-sense values that will just lead to a nightmare for the average dev to debug
|
fyi, this is going to miss this SDK cycle now, which I think it is fine, gives more time to test this over time, but still happy to make this the new baseline |
2 participants
Fixes #6780