spirv-val: Add Location Interpolation check #6002
Conversation
spencer-lunarg
force-pushed
the
spencer-lunarg-interpolation-cross
branch
from
92f3f10 to
e382fb5
Compare
spencer-lunarg
force-pushed
the
spencer-lunarg-interpolation-cross
branch
from
e382fb5 to
dc8cd21
Compare
spencer-lunarg
force-pushed
the
spencer-lunarg-interpolation-cross
branch
from
dc8cd21 to
22310bb
Compare
| EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_VULKAN_1_0)); | ||
| } | ||
|
|
||
| TEST_F(ValidateInterfacesTest, InterpolationOverlapBlockMixMembers) { |
There was a problem hiding this comment.
We have https://gitlab.khronos.org/vulkan/vulkan/-/issues/4200 open
Things like this "might" be invalid, but went more conservative with the validation in this PR because it is new, covering the "real" use case, and can always restrict more in the future (and we have tests already then!)
There was a problem hiding this comment.
Needs a test for the following type of case:
OpDecorate %in1 Location 0
OpDecorate %in1 Component 0
OpDecorate %in2 Location 0
OpDecorate %in2 Component 1
OpDecorate %in2 Centroid
I think the code would correctly consider the above invalid, but center interpolation isn't covered here.
| dec == spv::Decoration::Sample || dec == spv::Decoration::Centroid; | ||
| } | ||
|
|
||
| // Need to check Variables (or struct members) with explicit Locaiton decoration |
There was a problem hiding this comment.
| // Need to check Variables (or struct members) with explicit Locaiton decoration | |
| // Need to check Variables (or struct members) with explicit Location decoration |
| std::unordered_map<uint32_t, std::vector<spv::Decoration>> | ||
| input_interpolation; | ||
| std::unordered_map<uint32_t, std::vector<spv::Decoration>> | ||
| output_interpolation; |
There was a problem hiding this comment.
Seems unnecessary to have a vector of decorations. Just store the first interpolation decoration found for a location and if another is encountered issue an error immediately.
You may need to add tests for membership to delineate the center vs other interpolations.
| location = dec.params()[0]; | ||
| has_location = true; | ||
| } else if (IsInterpolationDecoration(decoration)) { | ||
| interpolation_decs.push_back(decoration); |
There was a problem hiding this comment.
This seems like it would allow:
OpMemberDecorate %struct 0 Location 0
OpMemberDecorate %struct 0 Sample
OpMemberDecorate %struct 0 Centroid
Is that caught elsewhere? Certainly it should be invalid.
I tried to quick apply https://gitlab.khronos.org/vulkan/vulkan/-/merge_requests/7183 but hit some issues (plan to add to KhronosGroup/Vulkan-Guide#292 after we settle on the answers)
I wrote tests, didn't try implementing it until I know what should fail or not
update - this is VU
VUID-StandaloneSpirv-Input-10604now