From e2646f5e9562e738daed967d48aae1e65225c955 Mon Sep 17 00:00:00 2001
From: Sven van Haastregt <sven.vanhaastregt@arm.com>
Date: Tue, 21 May 2024 19:02:17 +0200
Subject: [PATCH] spirv-val: Consider target env for OpReadClockKHR scope
 (#5681)

The Scope operand of `OpReadClockKHR` was always validated using the
Vulkan environment rules, which only allow `Subgroup` or `Device`.
For the OpenCL environment, `Workgroup` is also a valid Scope, so
`Workgroup` should not be rejected in the universal environment.

Guard the existing Scope check behind `spvIsVulkanEnv` and add a new
Scope check for the OpenCL environment.

Signed-off-by: Sven van Haastregt <sven.vanhaastregt@arm.com>
---
 source/val/validate_misc.cpp | 20 ++++++++++---
 test/val/val_misc_test.cpp   | 55 +++++++++++++++++++++++++++++++++++-
 2 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/source/val/validate_misc.cpp b/source/val/validate_misc.cpp
index d71fd2d26..a404134b6 100644
--- a/source/val/validate_misc.cpp
+++ b/source/val/validate_misc.cpp
@@ -50,10 +50,22 @@ spv_result_t ValidateShaderClock(ValidationState_t& _,
   bool is_int32 = false, is_const_int32 = false;
   uint32_t value = 0;
   std::tie(is_int32, is_const_int32, value) = _.EvalInt32IfConst(scope);
-  if (is_const_int32 && spv::Scope(value) != spv::Scope::Subgroup &&
-      spv::Scope(value) != spv::Scope::Device) {
-    return _.diag(SPV_ERROR_INVALID_DATA, inst)
-           << _.VkErrorID(4652) << "Scope must be Subgroup or Device";
+  if (is_const_int32) {
+    spv::Scope scope_val{value};
+    if (spvIsVulkanEnv(_.context()->target_env)) {
+      if (scope_val != spv::Scope::Subgroup &&
+          scope_val != spv::Scope::Device) {
+        return _.diag(SPV_ERROR_INVALID_DATA, inst)
+               << _.VkErrorID(4652) << "Scope must be Subgroup or Device";
+      }
+    } else if (spvIsOpenCLEnv(_.context()->target_env)) {
+      if (scope_val != spv::Scope::Workgroup &&
+          scope_val != spv::Scope::Subgroup &&
+          scope_val != spv::Scope::Device) {
+        return _.diag(SPV_ERROR_INVALID_DATA, inst)
+               << "Scope must be Subgroup, Workgroup, or Device";
+      }
+    }
   }
 
   // Result Type must be a 64 - bit unsigned integer type or
diff --git a/test/val/val_misc_test.cpp b/test/val/val_misc_test.cpp
index b0e46bf95..8c1f11dfe 100644
--- a/test/val/val_misc_test.cpp
+++ b/test/val/val_misc_test.cpp
@@ -222,7 +222,7 @@ OpReturn
 OpFunctionEnd)";
 
   CompileSuccessfully(spirv);
-  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
   EXPECT_THAT(getDiagnosticString(),
               HasSubstr("Scope must be Subgroup or Device"));
 }
@@ -251,6 +251,59 @@ OpFunctionEnd)";
               HasSubstr("Scope must be Subgroup or Device"));
 }
 
+std::string GenKernelClockSpirv(const std::string& scope) {
+  const std::string s = R"(
+OpCapability Kernel
+OpCapability Addresses
+OpCapability Int64
+OpCapability ShaderClockKHR
+OpExtension "SPV_KHR_shader_clock"
+OpMemoryModel Physical32 OpenCL
+OpEntryPoint Kernel %main "main"
+OpExecutionMode %main ContractionOff
+OpSource OpenCL_C 200000
+OpName %main "main"
+OpName %time1 "time1"
+%void = OpTypeVoid
+%3 = OpTypeFunction %void
+%ulong = OpTypeInt 64 0
+%uint = OpTypeInt 32 0
+%_ptr_Function_ulong = OpTypePointer Function %ulong
+%scope = OpConstant %uint )" +
+                        scope + R"(
+%main = OpFunction %void None %3
+%5 = OpLabel
+%time1 = OpVariable %_ptr_Function_ulong Function
+%11 = OpReadClockKHR %ulong %scope
+OpStore %time1 %11
+OpReturn
+OpFunctionEnd
+)";
+  return s;
+}
+
+TEST_F(ValidateMisc, KernelClockScopeDevice) {
+  CompileSuccessfully(GenKernelClockSpirv("1"), SPV_ENV_OPENCL_1_2);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_OPENCL_1_2));
+}
+
+TEST_F(ValidateMisc, KernelClockScopeWorkgroup) {
+  CompileSuccessfully(GenKernelClockSpirv("2"), SPV_ENV_OPENCL_1_2);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_OPENCL_1_2));
+}
+
+TEST_F(ValidateMisc, KernelClockScopeSubgroup) {
+  CompileSuccessfully(GenKernelClockSpirv("3"), SPV_ENV_OPENCL_1_2);
+  EXPECT_EQ(SPV_SUCCESS, ValidateInstructions(SPV_ENV_OPENCL_1_2));
+}
+
+TEST_F(ValidateMisc, KernelClockScopeInvalid) {
+  CompileSuccessfully(GenKernelClockSpirv("0"), SPV_ENV_OPENCL_1_2);
+  EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_OPENCL_1_2));
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Scope must be Subgroup, Workgroup, or Device"));
+}
+
 TEST_F(ValidateMisc, UndefVoid) {
   const std::string spirv = R"(
                OpCapability Shader