diff --git a/source/validate_instruction.cpp b/source/validate_instruction.cpp index 0537700d6..1d88c92ee 100644 --- a/source/validate_instruction.cpp +++ b/source/validate_instruction.cpp @@ -80,15 +80,12 @@ spv_capability_mask_t RequiredCapabilities(const AssemblyGrammar& grammar, if (SPV_SUCCESS == grammar.lookupOperand(type, operand, &operand_desc)) { result = operand_desc->capabilities; - // There's disagreement about whether mere mention of ClipDistance and - // CullDistance implies a requirement to declare their associated - // capabilities. Until the dust settles, turn off those checks. - // See https://github.com/KhronosGroup/SPIRV-Tools/issues/261 - // TODO(dneto): Once the final decision is made, fix this in a more - // permanent way, e.g. by generating Vulkan-specific operand tables that - // eliminate this capability dependency. - if (type == SPV_OPERAND_TYPE_BUILT_IN && - grammar.target_env() == SPV_ENV_VULKAN_1_0) { + // Mere mention of ClipDistance and CullDistance in a Builtin decoration + // does not require the associated capability. The use of such a variable + // value should trigger the capability requirement, but that's not + // implemented yet. This rule is independent of target environment. + // See https://github.com/KhronosGroup/SPIRV-Tools/issues/365 + if (type == SPV_OPERAND_TYPE_BUILT_IN) { result = result & (~(SPV_CAPABILITY_AS_MASK(SpvCapabilityClipDistance) | SPV_CAPABILITY_AS_MASK(SpvCapabilityCullDistance))); } diff --git a/test/Validate.Capability.cpp b/test/Validate.Capability.cpp index 769f64693..6c18a6dc7 100644 --- a/test/Validate.Capability.cpp +++ b/test/Validate.Capability.cpp @@ -117,6 +117,8 @@ using ValidateCapabilityV11 = spvtest::ValidateBase; // Always assembles using Vulkan 1.0. // TODO(dneto): Refactor all these tests to scale better across environments. using ValidateCapabilityVulkan10 = spvtest::ValidateBase; +// Always assembles using OpenGL 4.0. +using ValidateCapabilityOpenGL40 = spvtest::ValidateBase; TEST_F(ValidateCapability, Default) { const char str[] = R"( @@ -928,12 +930,15 @@ make_pair(string(kOpenCLMemoryModel) + make_pair(string(kOpenCLMemoryModel) + "OpDecorate %intt BuiltIn PointSize\n" "%intt = OpTypeInt 32 1\n", ShaderDependencies()), +// Just mentioning ClipDistance or CullDistance as a BuiltIn does +// not trigger the requirement for the associated capability. +// See https://github.com/KhronosGroup/SPIRV-Tools/issues/365 make_pair(string(kOpenCLMemoryModel) + "OpDecorate %intt BuiltIn ClipDistance\n" - "%intt = OpTypeInt 32 1\n", vector{"ClipDistance"}), + "%intt = OpTypeInt 32 1\n", AllCapabilities()), make_pair(string(kOpenCLMemoryModel) + "OpDecorate %intt BuiltIn CullDistance\n" - "%intt = OpTypeInt 32 1\n", vector{"CullDistance"}), + "%intt = OpTypeInt 32 1\n", AllCapabilities()), make_pair(string(kOpenCLMemoryModel) + "OpDecorate %intt BuiltIn VertexId\n" "%intt = OpTypeInt 32 1\n", ShaderDependencies()), @@ -1053,10 +1058,10 @@ make_pair(string(kOpenCLMemoryModel) + "%intt = OpTypeInt 32 1\n", ShaderDependencies()) )),); -// There's disagreement about whether mere mention of ClipDistance and -// CullDistance implies a requirement to declare their associated capabilities. -// Until the dust settles, turn off those checks. -// See https://github.com/KhronosGroup/SPIRV-Tools/issues/261 +// Ensure that mere mention of ClipDistance and CullDistance as +// BuiltIns does not trigger the requirement for the associated +// capability. +// See https://github.com/KhronosGroup/SPIRV-Tools/issues/365 INSTANTIATE_TEST_CASE_P(BuiltIn, ValidateCapabilityVulkan10, Combine( // Vulkan 1.0 is based on SPIR-V 1.0 @@ -1070,6 +1075,19 @@ make_pair(string(kGLSL450MemoryModel) + "%intt = OpTypeInt 32 1\n", AllV10Capabilities()) )),); +INSTANTIATE_TEST_CASE_P(BuiltIn, ValidateCapabilityOpenGL40, + Combine( + // OpenGL 4.0 is based on SPIR-V 1.0 + ValuesIn(AllV10Capabilities()), + Values( +make_pair(string(kGLSL450MemoryModel) + + "OpDecorate %intt BuiltIn ClipDistance\n" + "%intt = OpTypeInt 32 1\n", AllV10Capabilities()), +make_pair(string(kGLSL450MemoryModel) + + "OpDecorate %intt BuiltIn CullDistance\n" + "%intt = OpTypeInt 32 1\n", AllV10Capabilities()) +)),); + // TODO(umar): Selection Control // TODO(umar): Loop Control // TODO(umar): Function Control @@ -1168,13 +1186,20 @@ TEST_P(ValidateCapabilityV11, Capability) { TEST_P(ValidateCapabilityVulkan10, Capability) { const string test_code = MakeAssembly(GetParam()); - std::cout << test_code << std::endl; CompileSuccessfully(test_code, SPV_ENV_VULKAN_1_0); ASSERT_EQ(ExpectedResult(GetParam()), ValidateInstructions(SPV_ENV_VULKAN_1_0)) << test_code; } +TEST_P(ValidateCapabilityOpenGL40, Capability) { + const string test_code = MakeAssembly(GetParam()); + CompileSuccessfully(test_code, SPV_ENV_OPENGL_4_0); + ASSERT_EQ(ExpectedResult(GetParam()), + ValidateInstructions(SPV_ENV_OPENGL_4_0)) + << test_code; +} + TEST_F(ValidateCapability, SemanticsIdIsAnIdNotALiteral) { // From https://github.com/KhronosGroup/SPIRV-Tools/issues/248 // The validator was interpreting the memory semantics ID number