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 Vulkan decoration interface #4831

Merged
merged 2 commits into from
Jul 7, 2022
Merged

spirv-val: Add Vulkan decoration interface #4831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ee169b
spirv-val: Add Vulkan decoration interface
sjfricke Jun 28, 2022
0ce43b1
Improve error message
sjfricke Jun 29, 2022
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
50 changes: 50 additions & 0 deletions source/val/validate_decorations.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,56 @@ spv_result_t CheckDecorationsOfEntryPoints(ValidationState_t& vstate) {
++num_workgroup_variables_with_aliased;
}
}

if (spvIsVulkanEnv(vstate.context()->target_env)) {
const auto* models = vstate.GetExecutionModels(entry_point);
const bool has_frag =
models->find(SpvExecutionModelFragment) != models->end();
const bool has_vert =
models->find(SpvExecutionModelVertex) != models->end();
for (const auto& decoration :
vstate.id_decorations(var_instr->id())) {
if (decoration == SpvDecorationFlat ||
decoration == SpvDecorationNoPerspective ||
decoration == SpvDecorationSample ||
decoration == SpvDecorationCentroid) {
// VUID 04670 already validates these decorations are input/output
if (storage_class == SpvStorageClassInput &&
(models->size() > 1 || has_vert)) {
return vstate.diag(SPV_ERROR_INVALID_ID, var_instr)
<< vstate.VkErrorID(6202)
<< "OpEntryPoint interfaces variable must not be vertex "
"execution model with an input storage class for "
"Entry Point id "
<< entry_point << ".";
} else if (storage_class == SpvStorageClassOutput &&
(models->size() > 1 || has_frag)) {
return vstate.diag(SPV_ERROR_INVALID_ID, var_instr)
<< vstate.VkErrorID(6201)
<< "OpEntryPoint interfaces variable must not be "
"fragment "
"execution model with an output storage class for "
"Entry Point id "
<< entry_point << ".";
Comment on lines +823 to +844
Copy link

Choose a reason for hiding this comment

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

I don't understand the phrasing used here:

  • "must not be {vertex,fragment} execution model"
    • how can a variable be an execution model?
      (is there a missing "for" in the middle there?)
  • it (effectively) claims vertex shaders can't have inputs and fragment shaders can't have outputs?!
    • without looking up e.g. VUID-StandaloneSpirv-Flat-06201 in the spec there is no way at all to tell that the error is talking about decorations on interface variables, not their storage classes
      (let alone which decorations, i.e. Flat/NoPerspective/Sample/Centroid)

It almost looks like there's a place left in the message for printing the decoration name, but that was forgotten? (e.g. after "must not be" one could add "decorated with Flat", though the rest of the message needs a bit of tweaking too)

Copy link

Choose a reason for hiding this comment

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

Looking at the PR this replaces, its version of the message uses LogStringForDecoration and overall seems readable, unlike this PR.

Despite the GH username change, it looks like both PRs are by @sjfricke so I don't understand how the message regression happened.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I agree the error message is a little vague, can improve that! (unless you were planning on making the PR)

Sorry, I forgot about LogStringForDecoration ... I made both these PR a year apart and there were some spec changes in between that time too that was blocking the original PR.

}
}
}

const bool has_flat =
hasDecoration(var_instr->id(), SpvDecorationFlat, vstate);
if (has_frag && storage_class == SpvStorageClassInput && !has_flat &&
((vstate.IsFloatScalarType(type_id) &&
vstate.GetBitWidth(type_id) == 64) ||
vstate.IsIntScalarOrVectorType(type_id))) {
return vstate.diag(SPV_ERROR_INVALID_ID, var_instr)
<< vstate.VkErrorID(4744)
<< "Fragment OpEntryPoint operand "
<< interface << " with Input interfaces with integer or "
"float type must have a Flat decoration "
"for Entry Point id "
<< entry_point << ".";
}
}
}
if (num_builtin_block_inputs > 1 || num_builtin_block_outputs > 1) {
return vstate.diag(SPV_ERROR_INVALID_BINARY,
Expand Down
8 changes: 7 additions & 1 deletion source/val/validation_state.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1878,7 +1878,7 @@ std::string ValidationState_t::VkErrorID(uint32_t id,
case 4677:
return VUID_WRAP(VUID-StandaloneSpirv-Invariant-04677);
case 4680:
return VUID_WRAP( VUID-StandaloneSpirv-OpTypeRuntimeArray-04680);
return VUID_WRAP(VUID-StandaloneSpirv-OpTypeRuntimeArray-04680);
case 4682:
return VUID_WRAP(VUID-StandaloneSpirv-OpControlBarrier-04682);
case 6426:
Expand All @@ -1903,6 +1903,8 @@ std::string ValidationState_t::VkErrorID(uint32_t id,
return VUID_WRAP(VUID-StandaloneSpirv-OpMemoryBarrier-04733);
case 4734:
return VUID_WRAP(VUID-StandaloneSpirv-OpVariable-04734);
case 4744:
return VUID_WRAP(VUID-StandaloneSpirv-Flat-04744);
case 4777:
return VUID_WRAP(VUID-StandaloneSpirv-OpImage-04777);
case 4780:
Expand All @@ -1919,6 +1921,10 @@ std::string ValidationState_t::VkErrorID(uint32_t id,
return VUID_WRAP(VUID-StandaloneSpirv-Location-04918);
case 4919:
return VUID_WRAP(VUID-StandaloneSpirv-Location-04919);
case 6201:
return VUID_WRAP(VUID-StandaloneSpirv-Flat-06201);
case 6202:
return VUID_WRAP(VUID-StandaloneSpirv-Flat-06202);
case 6214:
return VUID_WRAP(VUID-StandaloneSpirv-OpTypeImage-06214);
case 6491:
Expand Down
5 changes: 5 additions & 0 deletions test/val/val_builtins_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,11 @@ CodeGenerator GetVariableCodeGenerator(const char* const built_in,
generator.before_types_ = "OpDecorate %built_in_var BuiltIn ";
generator.before_types_ += built_in;
generator.before_types_ += "\n";
if ((0 == std::strcmp(storage_class, "Input")) &&
(0 == std::strcmp(execution_model, "Fragment"))) {
// ensure any needed input types that might require Flat
generator.before_types_ += "OpDecorate %built_in_var Flat\n";
}

std::ostringstream after_types;
if (InitializerRequired(storage_class)) {
Expand Down
286 changes: 286 additions & 0 deletions test/val/val_decoration_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ struct TestResult {
};

using ValidateDecorations = spvtest::ValidateBase<bool>;
using ValidateDecorationString = spvtest::ValidateBase<std::string>;
using ValidateVulkanCombineDecorationResult =
spvtest::ValidateBase<std::tuple<const char*, const char*, TestResult>>;

Expand Down Expand Up @@ -8409,6 +8410,291 @@ TEST_F(ValidateDecorations, RelaxedPrecisionDecorationOnStructMember) {
EXPECT_EQ(SPV_SUCCESS, ValidateAndRetrieveValidationState(env));
}

TEST_F(ValidateDecorations, VulkanFlatMultipleInterfaceGood) {
std::string spirv = R"(
OpCapability Shader
OpCapability Geometry
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %main "main" %layer %gl_Layer
OpExecutionMode %main OriginUpperLeft
OpSource GLSL 450
OpDecorate %layer Location 0
OpDecorate %gl_Layer Flat
OpDecorate %gl_Layer BuiltIn Layer
%void = OpTypeVoid
%3 = OpTypeFunction %void
%int = OpTypeInt 32 1
%_ptr_Output_int = OpTypePointer Output %int
%layer = OpVariable %_ptr_Output_int Output
%_ptr_Input_int = OpTypePointer Input %int
%gl_Layer = OpVariable %_ptr_Input_int Input
%main = OpFunction %void None %3
%5 = OpLabel
%11 = OpLoad %int %gl_Layer
OpStore %layer %11
OpReturn
OpFunctionEnd
)";

CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
EXPECT_EQ(SPV_SUCCESS,
ValidateAndRetrieveValidationState(SPV_ENV_VULKAN_1_0));
}

TEST_F(ValidateDecorations, VulkanFlatMultipleInterfaceBad) {
std::string spirv = R"(
OpCapability Shader
OpCapability Geometry
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %main "main" %layer %gl_Layer
OpExecutionMode %main OriginUpperLeft
OpSource GLSL 450
OpDecorate %layer Location 0
OpDecorate %gl_Layer BuiltIn Layer
%void = OpTypeVoid
%3 = OpTypeFunction %void
%int = OpTypeInt 32 1
%_ptr_Output_int = OpTypePointer Output %int
%layer = OpVariable %_ptr_Output_int Output
%_ptr_Input_int = OpTypePointer Input %int
%gl_Layer = OpVariable %_ptr_Input_int Input
%main = OpFunction %void None %3
%5 = OpLabel
%11 = OpLoad %int %gl_Layer
OpStore %layer %11
OpReturn
OpFunctionEnd
)";

CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
EXPECT_EQ(SPV_ERROR_INVALID_ID,
ValidateAndRetrieveValidationState(SPV_ENV_VULKAN_1_0));
EXPECT_THAT(getDiagnosticString(),
AnyVUID("VUID-StandaloneSpirv-Flat-04744"));
EXPECT_THAT(
getDiagnosticString(),
HasSubstr(
"Fragment OpEntryPoint operand 4 with Input interfaces with integer "
"or float type must have a Flat decoration for Entry Point id 2."));
}

TEST_F(ValidateDecorations, VulkanNoFlatFloat32) {
std::string spirv = R"(
OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %main "main" %in
OpExecutionMode %main OriginUpperLeft
OpSource GLSL 450
OpDecorate %in Location 0
%void = OpTypeVoid
%3 = OpTypeFunction %void
%float = OpTypeFloat 32
%_ptr_Function_float = OpTypePointer Function %float
%_ptr_Input_float = OpTypePointer Input %float
%in = OpVariable %_ptr_Input_float Input
%main = OpFunction %void None %3
%5 = OpLabel
%b = OpVariable %_ptr_Function_float Function
%11 = OpLoad %float %in
OpStore %b %11
OpReturn
OpFunctionEnd
)";

CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
EXPECT_EQ(SPV_SUCCESS,
ValidateAndRetrieveValidationState(SPV_ENV_VULKAN_1_0));
}

TEST_F(ValidateDecorations, VulkanNoFlatFloat64) {
std::string spirv = R"(
OpCapability Shader
OpCapability Float64
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %main "main" %in
OpExecutionMode %main OriginUpperLeft
OpSource GLSL 450
OpDecorate %in Location 0
%void = OpTypeVoid
%3 = OpTypeFunction %void
%double = OpTypeFloat 64
%_ptr_Function_double = OpTypePointer Function %double
%_ptr_Input_double = OpTypePointer Input %double
%in = OpVariable %_ptr_Input_double Input
%main = OpFunction %void None %3
%5 = OpLabel
%b = OpVariable %_ptr_Function_double Function
%11 = OpLoad %double %in
OpStore %b %11
OpReturn
OpFunctionEnd
)";

CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
EXPECT_EQ(SPV_ERROR_INVALID_ID,
ValidateAndRetrieveValidationState(SPV_ENV_VULKAN_1_0));
EXPECT_THAT(getDiagnosticString(),
AnyVUID("VUID-StandaloneSpirv-Flat-04744"));
EXPECT_THAT(
getDiagnosticString(),
HasSubstr(
"Fragment OpEntryPoint operand 3 with Input interfaces with integer "
"or float type must have a Flat decoration for Entry Point id 2."));
}

TEST_F(ValidateDecorations, VulkanNoFlatVectorFloat64) {
std::string spirv = R"(
OpCapability Shader
OpCapability Float64
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %main "main" %in
OpExecutionMode %main OriginUpperLeft
OpSource GLSL 450
OpDecorate %in Location 0
%void = OpTypeVoid
%3 = OpTypeFunction %void
%double = OpTypeFloat 64
%v2double = OpTypeVector %double 2
%_ptr_Function_v2double = OpTypePointer Function %v2double
%_ptr_Input_v2double = OpTypePointer Input %v2double
%in = OpVariable %_ptr_Input_v2double Input
%main = OpFunction %void None %3
%5 = OpLabel
%b = OpVariable %_ptr_Function_v2double Function
%11 = OpLoad %v2double %in
OpStore %b %11
OpReturn
OpFunctionEnd
)";

CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
EXPECT_EQ(SPV_SUCCESS,
ValidateAndRetrieveValidationState(SPV_ENV_VULKAN_1_0));
}

TEST_F(ValidateDecorations, VulkanNoFlatIntVector) {
std::string spirv = R"(
OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %main "main" %in
OpExecutionMode %main OriginUpperLeft
OpSource GLSL 450
OpDecorate %in Location 0
%void = OpTypeVoid
%3 = OpTypeFunction %void
%int = OpTypeInt 32 1
%v2int = OpTypeVector %int 2
%_ptr_Function_v2int = OpTypePointer Function %v2int
%_ptr_Input_v2int = OpTypePointer Input %v2int
%in = OpVariable %_ptr_Input_v2int Input
%main = OpFunction %void None %3
%5 = OpLabel
%b = OpVariable %_ptr_Function_v2int Function
%12 = OpLoad %v2int %in
OpStore %b %12
OpReturn
OpFunctionEnd
)";

CompileSuccessfully(spirv, SPV_ENV_VULKAN_1_0);
EXPECT_EQ(SPV_ERROR_INVALID_ID,
ValidateAndRetrieveValidationState(SPV_ENV_VULKAN_1_0));
EXPECT_THAT(getDiagnosticString(),
AnyVUID("VUID-StandaloneSpirv-Flat-04744"));
EXPECT_THAT(
getDiagnosticString(),
HasSubstr(
"Fragment OpEntryPoint operand 3 with Input interfaces with integer "
"or float type must have a Flat decoration for Entry Point id 2."));
}

TEST_P(ValidateDecorationString, VulkanOutputInvalidInterface) {
const std::string decoration = GetParam();
std::stringstream ss;
ss << R"(
OpCapability Shader
OpCapability SampleRateShading
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Fragment %main "main" %out
OpExecutionMode %main OriginUpperLeft
OpSource GLSL 450
OpDecorate %out )"
<< decoration << R"(
OpDecorate %out Location 0
%void = OpTypeVoid
%3 = OpTypeFunction %void
%int = OpTypeInt 32 1
%_ptr_Output_int = OpTypePointer Output %int
%out = OpVariable %_ptr_Output_int Output
%int_1 = OpConstant %int 1
%main = OpFunction %void None %3
%5 = OpLabel
OpStore %out %int_1
OpReturn
OpFunctionEnd
)";

CompileSuccessfully(ss.str(), SPV_ENV_VULKAN_1_0);
ASSERT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
EXPECT_THAT(getDiagnosticString(),
AnyVUID("VUID-StandaloneSpirv-Flat-06201"));
EXPECT_THAT(
getDiagnosticString(),
HasSubstr(
"OpEntryPoint interfaces variable must not be fragment execution "
"model with an output storage class for Entry Point id 2."));
}

TEST_P(ValidateDecorationString, VulkanVertexInputInvalidInterface) {
const std::string decoration = GetParam();
std::stringstream ss;
ss << R"(
OpCapability Shader
OpCapability SampleRateShading
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint Vertex %main "main" %out %in
OpSource GLSL 450
OpDecorate %in )"
<< decoration << R"(
OpDecorate %out Location 0
OpDecorate %in Location 0
%void = OpTypeVoid
%3 = OpTypeFunction %void
%int = OpTypeInt 32 1
%_ptr_Output_int = OpTypePointer Output %int
%out = OpVariable %_ptr_Output_int Output
%_ptr_Input_int = OpTypePointer Input %int
%in = OpVariable %_ptr_Input_int Input
%main = OpFunction %void None %3
%5 = OpLabel
%11 = OpLoad %int %in
OpStore %out %11
OpReturn
OpFunctionEnd
)";

CompileSuccessfully(ss.str(), SPV_ENV_VULKAN_1_0);
ASSERT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
EXPECT_THAT(getDiagnosticString(),
AnyVUID("VUID-StandaloneSpirv-Flat-06202"));
EXPECT_THAT(
getDiagnosticString(),
HasSubstr("OpEntryPoint interfaces variable must not be vertex execution "
"model with an input storage class for Entry Point id 2."));
}

INSTANTIATE_TEST_SUITE_P(FragmentInputInterface, ValidateDecorationString,
::testing::Values("Flat", "NoPerspective", "Sample",
"Centroid"));

} // namespace
} // namespace val
} // namespace spvtools
Loading