From 06581f5ce6cb636ed1c6611da371656c26a1d871 Mon Sep 17 00:00:00 2001
From: David Neto <dneto@google.com>
Date: Thu, 7 Jul 2016 17:03:22 -0400
Subject: [PATCH] Turn off ClipDistance CullDistance cap checks for Vulkan

Turn them off until resolution of the debate over how they should be checked.
Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/261
---
 CHANGES                         |  2 ++
 source/validate_instruction.cpp | 24 ++++++++++++++++++++----
 test/Validate.Capability.cpp    | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/CHANGES b/CHANGES
index 615605054..7074a50d2 100644
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,8 @@ Revision history for SPIRV-Tools
 
 v2016.1-dev 2016-07-04
  - Start v2016.1
+ - Fix https://github.com/KhronosGroup/SPIRV-Tools/issues/261
+   Turn off ClipDistance and CullDistance capability checks for Vulkan.
 
 v2016.0 2016-07-04
 
diff --git a/source/validate_instruction.cpp b/source/validate_instruction.cpp
index 243ca0ff4..0537700d6 100644
--- a/source/validate_instruction.cpp
+++ b/source/validate_instruction.cpp
@@ -74,11 +74,27 @@ spv_result_t CapabilityError(ValidationState_t& _, int which_operand,
 spv_capability_mask_t RequiredCapabilities(const AssemblyGrammar& grammar,
                                            spv_operand_type_t type,
                                            uint32_t operand) {
+  spv_capability_mask_t result = 0;
   spv_operand_desc operand_desc;
-  if (SPV_SUCCESS == grammar.lookupOperand(type, operand, &operand_desc))
-    return operand_desc->capabilities;
-  else
-    return 0;
+
+  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) {
+      result = result & (~(SPV_CAPABILITY_AS_MASK(SpvCapabilityClipDistance) |
+                           SPV_CAPABILITY_AS_MASK(SpvCapabilityCullDistance)));
+    }
+  }
+
+  return result;
 }
 
 }  // namespace
diff --git a/test/Validate.Capability.cpp b/test/Validate.Capability.cpp
index e635ba6c1..5083a12a8 100644
--- a/test/Validate.Capability.cpp
+++ b/test/Validate.Capability.cpp
@@ -114,6 +114,10 @@ using ValidateCapability = spvtest::ValidateBase<CapTestParameter>;
 // Always assembles using v1.1.
 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>;
+
 TEST_F(ValidateCapability, Default) {
   const char str[] = R"(
             OpCapability Kernel
@@ -1057,6 +1061,23 @@ 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
+INSTANTIATE_TEST_CASE_P(BuiltIn, ValidateCapabilityVulkan10,
+                        Combine(
+                            // Vulkan 1.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
@@ -1134,6 +1155,8 @@ bool Exists(const std::string& capability, spv_target_env env) {
                             capability.size(), &dummy);
 }
 
+
+
 TEST_P(ValidateCapability, Capability) {
   const string capability = Capability(GetParam());
   spv_target_env env =
@@ -1153,6 +1176,15 @@ TEST_P(ValidateCapabilityV11, Capability) {
       << test_code;
 }
 
+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_F(ValidateCapability, SemanticsIdIsAnIdNotALiteral) {
   // From https://github.com/KhronosGroup/SPIRV-Tools/issues/248
   // The validator was interpreting the memory semantics ID number