Skip to content

Commit

Permalink
Patch location validation VUIDs (#4664)
Browse files Browse the repository at this point in the history
* spirv-val: Add Vulkan Location VUID

* Simplify: interface checking already does all the work

Co-authored-by: sfricke_samsung <s.fricke@samsung.com>
  • Loading branch information
dneto0 and sfricke-samsung authored Dec 16, 2021
1 parent 7d76881 commit 5d0e324
Show file tree
Hide file tree
Showing 7 changed files with 385 additions and 9 deletions.
2 changes: 1 addition & 1 deletion source/val/validate_decorations.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@ spv_result_t CheckBuiltInVariable(uint32_t var_id, ValidationState_t& vstate) {
if (d.dec_type() == SpvDecorationLocation ||
d.dec_type() == SpvDecorationComponent) {
return vstate.diag(SPV_ERROR_INVALID_ID, vstate.FindDef(var_id))
<< "A BuiltIn variable (id " << var_id
<< vstate.VkErrorID(4915) << "A BuiltIn variable (id " << var_id
<< ") cannot have any Location or Component decorations";
}
}
Expand Down
10 changes: 7 additions & 3 deletions source/val/validate_interfaces.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ spv_result_t NumConsumedLocations(ValidationState_t& _, const Instruction* type,
// Members cannot have location decorations at this point.
if (_.HasDecoration(type->id(), SpvDecorationLocation)) {
return _.diag(SPV_ERROR_INVALID_DATA, type)
<< "Members cannot be assigned a location";
<< _.VkErrorID(4918) << "Members cannot be assigned a location";
}

// Structs consume locations equal to the sum of the locations consumed
Expand Down Expand Up @@ -326,8 +326,9 @@ spv_result_t GetLocationsForVariable(
// Only block-decorated structs don't need a location on the variable.
const bool is_block = _.HasDecoration(type_id, SpvDecorationBlock);
if (!has_location && !is_block) {
const auto vuid = (type->opcode() == SpvOpTypeStruct) ? 4917 : 4916;
return _.diag(SPV_ERROR_INVALID_DATA, variable)
<< "Variable must be decorated with a location";
<< _.VkErrorID(vuid) << "Variable must be decorated with a location";
}

const std::string storage_class = is_output ? "output" : "input";
Expand Down Expand Up @@ -411,7 +412,7 @@ spv_result_t GetLocationsForVariable(
auto where = member_locations.find(i - 1);
if (where == member_locations.end()) {
return _.diag(SPV_ERROR_INVALID_DATA, type)
<< "Member index " << i - 1
<< _.VkErrorID(4919) << "Member index " << i - 1
<< " is missing a location assignment";
}

Expand Down Expand Up @@ -476,6 +477,9 @@ spv_result_t ValidateLocations(ValidationState_t& _,
const Instruction* entry_point) {
// According to Vulkan 14.1 only the following execution models have
// locations assigned.
// TODO(dneto): SPV_NV_ray_tracing also uses locations on interface variables,
// in other shader stages. Similarly, the *provisional* version of
// SPV_KHR_ray_tracing did as well, but not the final version.
switch (entry_point->GetOperandAs<SpvExecutionModel>(0)) {
case SpvExecutionModelVertex:
case SpvExecutionModelTessellationControl:
Expand Down
19 changes: 19 additions & 0 deletions source/val/validation_state.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,15 @@ void ValidationState_t::RegisterCapability(SpvCapability cap) {
features_.variable_pointers_storage_buffer = true;
break;
default:
// TODO(dneto): For now don't validate SPV_NV_ray_tracing, which uses
// capability SpvCapabilityRayTracingNV.
// SpvCapabilityRayTracingProvisionalKHR would need the same treatment.
// One of the differences going from SPV_KHR_ray_tracing from
// provisional to final spec was the provisional spec uses Locations
// for variables in certain storage classes, just like the
// SPV_NV_ray_tracing extension. So it mimics the NVIDIA extension.
// The final SPV_KHR_ray_tracing uses a different capability token
// number, so it doesn't fall into this case.
break;
}
}
Expand Down Expand Up @@ -1875,6 +1884,16 @@ std::string ValidationState_t::VkErrorID(uint32_t id,
return VUID_WRAP(VUID-StandaloneSpirv-OpMemoryBarrier-04733);
case 4780:
return VUID_WRAP(VUID-StandaloneSpirv-Result-04780);
case 4915:
return VUID_WRAP(VUID-StandaloneSpirv-Location-04915);
case 4916:
return VUID_WRAP(VUID-StandaloneSpirv-Location-04916);
case 4917:
return VUID_WRAP(VUID-StandaloneSpirv-Location-04917);
case 4918:
return VUID_WRAP(VUID-StandaloneSpirv-Location-04918);
case 4919:
return VUID_WRAP(VUID-StandaloneSpirv-Location-04919);
default:
return ""; // unknown id
}
Expand Down
Loading

0 comments on commit 5d0e324

Please sign in to comment.