Relax ClipDistance, CullDistance capability check in all environments

Fixes KhronosGroup#365
dneto0 · Aug 23, 2016 · 422f307 · 422f307
1 parent ccabcc4
commit 422f307
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 15 deletions.
diff --git 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
@@ -117,6 +117,8 @@ using ValidateCapabilityV11 = spvtest::ValidateBase<CapTestParameter>;
 // Always assembles using Vulkan 1.0.
 // TODO(dneto): Refactor all these tests to scale better across environments.
 using ValidateCapabilityVulkan10 = spvtest::ValidateBase<CapTestParameter>;
+// Always assembles using OpenGL 4.0.
+using ValidateCapabilityOpenGL40 = spvtest::ValidateBase<CapTestParameter>;
 
 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<string>{"ClipDistance"}),
+          "%intt = OpTypeInt 32 1\n", AllCapabilities()),
 make_pair(string(kOpenCLMemoryModel) +
           "OpDecorate %intt BuiltIn CullDistance\n"
-          "%intt = OpTypeInt 32 1\n", vector<string>{"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
@@ -1175,6 +1193,15 @@ TEST_P(ValidateCapabilityVulkan10, Capability) {
       << test_code;
 }
 
+TEST_P(ValidateCapabilityOpenGL40, Capability) {
+  const string test_code = MakeAssembly(GetParam());
+  std::cout << test_code << std::endl;
+  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