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.

Sign up for GitHub

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

spirv-val: Add Location Interpolation check #6002

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

spirv-val: Add Location Interpolation check #6002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 69 additions & 3 deletions source/val/validate_interfaces.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,61 @@ spv_result_t GetLocationsForVariable(
return SPV_SUCCESS;
}

bool IsInterpolationDecoration(spv::Decoration dec) {
return dec == spv::Decoration::PerVertexKHR || dec == spv::Decoration::Flat ||
dec == spv::Decoration::NoPerspective ||
dec == spv::Decoration::Sample || dec == spv::Decoration::Centroid;
}

// Need to check Variables (or struct members) with explicit Locaiton decoration
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Need to check Variables (or struct members) with explicit Locaiton decoration
// Need to check Variables (or struct members) with explicit Location decoration

// don't mix interpolation decorations
spv_result_t ValidateLocationInterpolation(
ValidationState_t& _, bool is_input, const Instruction* variable,
std::unordered_map<uint32_t, std::vector<spv::Decoration>>&
interpolation_map) {
uint32_t location = 0;
bool has_location = false;
std::vector<spv::Decoration> interpolation_decs;
for (auto& dec : _.id_decorations(variable->id())) {
spv::Decoration decoration = dec.dec_type();
if (decoration == spv::Decoration::Location) {
location = dec.params()[0];
has_location = true;
} else if (IsInterpolationDecoration(decoration)) {
interpolation_decs.push_back(decoration);
}
}

// Look for Location in a Block decorated Struct
if (!has_location) {
auto ptr_type_id = variable->GetOperandAs<uint32_t>(0);
auto ptr_type = _.FindDef(ptr_type_id);
auto type_id = ptr_type->GetOperandAs<uint32_t>(2);
for (auto& dec : _.id_decorations(type_id)) {
spv::Decoration decoration = dec.dec_type();
if (decoration == spv::Decoration::Location) {
location = dec.params()[0];
has_location = true;
} else if (IsInterpolationDecoration(decoration)) {
interpolation_decs.push_back(decoration);
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 allow:

OpMemberDecorate %struct 0 Location 0
OpMemberDecorate %struct 0 Sample
OpMemberDecorate %struct 0 Centroid

Is that caught elsewhere? Certainly it should be invalid.

}
}
}

if (has_location) {
auto it = interpolation_map.find(location);
if (it == interpolation_map.end()) {
interpolation_map[location] = interpolation_decs;
} else if (interpolation_map[location] != interpolation_decs) {
return _.diag(SPV_ERROR_INVALID_DATA, variable)
<< _.VkErrorID(10604) << (is_input ? "input" : "output")
<< " Location " << location
<< " has conflicting Interpolation decorations";
}
}
return SPV_SUCCESS;
}

spv_result_t ValidateLocations(ValidationState_t& _,
const Instruction* entry_point) {
// According to Vulkan 14.1 only the following execution models have
Expand All @@ -554,6 +609,12 @@ spv_result_t ValidateLocations(ValidationState_t& _,
std::unordered_set<uint32_t> patch_locations_index0;
std::unordered_set<uint32_t> patch_locations_index1;
std::unordered_set<uint32_t> seen;

std::unordered_map<uint32_t, std::vector<spv::Decoration>>
input_interpolation;
std::unordered_map<uint32_t, std::vector<spv::Decoration>>
output_interpolation;
Comment on lines +613 to +616
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


for (uint32_t i = 3; i < entry_point->operands().size(); ++i) {
auto interface_id = entry_point->GetOperandAs<uint32_t>(i);
auto interface_var = _.FindDef(interface_id);
Expand Down Expand Up @@ -590,12 +651,17 @@ spv_result_t ValidateLocations(ValidationState_t& _,
continue;
}

auto locations = (storage_class == spv::StorageClass::Input)
? &input_locations
: &output_locations_index0;
bool is_input = storage_class == spv::StorageClass::Input;
auto locations = is_input ? &input_locations : &output_locations_index0;
if (auto error = GetLocationsForVariable(
_, entry_point, interface_var, locations, &output_locations_index1))
return error;

auto& interpolation_map =
is_input ? input_interpolation : output_interpolation;
if (auto error = ValidateLocationInterpolation(_, is_input, interface_var,
interpolation_map))
return error;
}

return SPV_SUCCESS;
Expand Down
2 changes: 2 additions & 0 deletions source/val/validation_state.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2536,6 +2536,8 @@ std::string ValidationState_t::VkErrorID(uint32_t id,
return VUID_WRAP(VUID-RuntimeSpirv-Offset-10213);
case 10583:
return VUID_WRAP(VUID-StandaloneSpirv-Component-10583);
case 10604:
return VUID_WRAP(VUID-StandaloneSpirv-Input-10604);
default:
return ""; // unknown id
}
Expand Down
Loading