Repository: KhronosGroup/SPIRV-Tools (language: python)

Issue:
spirv-opt: OpControlBarrier was removed when using barriers in mesh/task stages
Hello,

I was adding SPV_EXT_mesh_shader support for DXC : https://github.com/microsoft/DirectXShaderCompiler/pull/4725

When I tried to compile a HLSL mesh shader which contains the barrier call _GroupMemoryBarrierWithGroupSync()_ (target environment is Vulkan1.3), I received the following warning message:

>warning : SPIR-V legalization: Removing ControlBarrier instruction because of incompatible execution model.

And the _OpControlBarrier_ was removed from the output.

It brings me to the code block in SPIRV-Tools\source\opt\replace_invalid_opc.cpp:
```
          if (model != spv::ExecutionModel::TessellationControl &&
              model != spv::ExecutionModel::GLCompute)
          {
              if (inst->opcode() == spv::Op::OpControlBarrier)
              {
                  assert(model != spv::ExecutionModel::Kernel &&
                   "Expecting to be working on a shader module.");
                  replace = true;
              }
        }
```

Quoting from the SPIRV spec
>Before version 1.3, it is only valid to use this instruction with TessellationControl, GLCompute, or Kernel [execution models](https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#Execution_Model). There is no such restriction starting with version 1.3.

Therefore, _OpControlBarrier_ should be kept when the target environment is SPIRV1.3 or above.


Discussion / hints:

Fix patch:
diff --git a/source/opt/ir_context.h b/source/opt/ir_context.h
index 35075de171..8419ee7e95 100644
--- a/source/opt/ir_context.h
+++ b/source/opt/ir_context.h
@@ -645,6 +645,17 @@ class IRContext {
   // all have the same stage.
   spv::ExecutionModel GetStage();
 
+  // Returns true of the current target environment is at least that of the
+  // given environment.
+  bool IsTargetEnvAtLeast(spv_target_env env) {
+    // A bit of a hack. We assume that the target environments are appended to
+    // the enum, so that there is an appropriate order.
+    return syntax_context_->target_env >= env;
+  }
+
+  // Return the target environment for the current context.
+  spv_target_env GetTargetEnv() const { return syntax_context_->target_env; }
+
  private:
   // Builds the def-use manager from scratch, even if it was already valid.
   void BuildDefUseManager() {
diff --git a/source/opt/replace_invalid_opc.cpp b/source/opt/replace_invalid_opc.cpp
index 214097398d..1b97c0e84f 100644
--- a/source/opt/replace_invalid_opc.cpp
+++ b/source/opt/replace_invalid_opc.cpp
@@ -86,7 +86,8 @@ bool ReplaceInvalidOpcodePass::RewriteFunction(Function* function,
         }
 
         if (model != spv::ExecutionModel::TessellationControl &&
-            model != spv::ExecutionModel::GLCompute) {
+            model != spv::ExecutionModel::GLCompute &&
+            !context()->IsTargetEnvAtLeast(SPV_ENV_UNIVERSAL_1_3)) {
           if (inst->opcode() == spv::Op::OpControlBarrier) {
             assert(model != spv::ExecutionModel::Kernel &&
                    "Expecting to be working on a shader module.");


Test patch:
diff --git a/test/opt/ir_context_test.cpp b/test/opt/ir_context_test.cpp
index 86a3f4596e..28cd0d5e92 100644
--- a/test/opt/ir_context_test.cpp
+++ b/test/opt/ir_context_test.cpp
@@ -1149,6 +1149,91 @@ OpFunctionEnd)";
             20);
 }
 
+struct TargetEnvCompareTestData {
+  spv_target_env later_env, earlier_env;
+};
+
+using TargetEnvCompareTest = ::testing::TestWithParam<TargetEnvCompareTestData>;
+
+TEST_P(TargetEnvCompareTest, Case) {
+  // If new environments are added, then we must update the list of tests.
+  ASSERT_EQ(SPV_ENV_VULKAN_1_3 + 1, SPV_ENV_MAX);
+
+  const auto& tc = GetParam();
+
+  std::unique_ptr<Module> module(new Module());
+  IRContext localContext(tc.later_env, std::move(module),
+                         spvtools::MessageConsumer());
+  EXPECT_TRUE(localContext.IsTargetEnvAtLeast(tc.earlier_env));
+
+  if (tc.earlier_env != tc.later_env) {
+    std::unique_ptr<Module> module(new Module());
+    IRContext localContext(tc.earlier_env, std::move(module),
+                           spvtools::MessageConsumer());
+    EXPECT_FALSE(localContext.IsTargetEnvAtLeast(tc.later_env));
+  }
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    TestCase, TargetEnvCompareTest,
+    ::testing::Values(
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_0, SPV_ENV_UNIVERSAL_1_0},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_1, SPV_ENV_UNIVERSAL_1_0},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_2, SPV_ENV_UNIVERSAL_1_0},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_3, SPV_ENV_UNIVERSAL_1_0},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_4, SPV_ENV_UNIVERSAL_1_0},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_5, SPV_ENV_UNIVERSAL_1_0},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_6, SPV_ENV_UNIVERSAL_1_0},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_1, SPV_ENV_UNIVERSAL_1_1},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_2, SPV_ENV_UNIVERSAL_1_1},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_3, SPV_ENV_UNIVERSAL_1_1},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_4, SPV_ENV_UNIVERSAL_1_1},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_5, SPV_ENV_UNIVERSAL_1_1},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_6, SPV_ENV_UNIVERSAL_1_1},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_2, SPV_ENV_UNIVERSAL_1_2},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_3, SPV_ENV_UNIVERSAL_1_2},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_4, SPV_ENV_UNIVERSAL_1_2},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_5, SPV_ENV_UNIVERSAL_1_2},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_6, SPV_ENV_UNIVERSAL_1_2},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_3, SPV_ENV_UNIVERSAL_1_3},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_4, SPV_ENV_UNIVERSAL_1_3},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_5, SPV_ENV_UNIVERSAL_1_3},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_6, SPV_ENV_UNIVERSAL_1_3},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_4, SPV_ENV_UNIVERSAL_1_4},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_5, SPV_ENV_UNIVERSAL_1_4},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_6, SPV_ENV_UNIVERSAL_1_4},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_5, SPV_ENV_UNIVERSAL_1_5},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_6, SPV_ENV_UNIVERSAL_1_5},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_6, SPV_ENV_UNIVERSAL_1_6},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_0, SPV_ENV_UNIVERSAL_1_0},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_1, SPV_ENV_VULKAN_1_0},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_2, SPV_ENV_VULKAN_1_0},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_3, SPV_ENV_VULKAN_1_0},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_4, SPV_ENV_VULKAN_1_0},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_5, SPV_ENV_VULKAN_1_0},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_6, SPV_ENV_VULKAN_1_0},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_1, SPV_ENV_UNIVERSAL_1_0},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_1, SPV_ENV_UNIVERSAL_1_1},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_1, SPV_ENV_UNIVERSAL_1_2},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_1, SPV_ENV_UNIVERSAL_1_3},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_4, SPV_ENV_VULKAN_1_1},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_5, SPV_ENV_VULKAN_1_1},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_6, SPV_ENV_VULKAN_1_1},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_2, SPV_ENV_UNIVERSAL_1_0},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_2, SPV_ENV_UNIVERSAL_1_1},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_2, SPV_ENV_UNIVERSAL_1_2},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_2, SPV_ENV_UNIVERSAL_1_3},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_2, SPV_ENV_UNIVERSAL_1_4},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_2, SPV_ENV_UNIVERSAL_1_5},
+        TargetEnvCompareTestData{SPV_ENV_UNIVERSAL_1_6, SPV_ENV_VULKAN_1_2},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_3, SPV_ENV_UNIVERSAL_1_0},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_3, SPV_ENV_UNIVERSAL_1_1},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_3, SPV_ENV_UNIVERSAL_1_2},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_3, SPV_ENV_UNIVERSAL_1_3},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_3, SPV_ENV_UNIVERSAL_1_4},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_3, SPV_ENV_UNIVERSAL_1_5},
+        TargetEnvCompareTestData{SPV_ENV_VULKAN_1_3, SPV_ENV_UNIVERSAL_1_6}));
+
 }  // namespace
 }  // namespace opt
 }  // namespace spvtools
diff --git a/test/opt/replace_invalid_opc_test.cpp b/test/opt/replace_invalid_opc_test.cpp
index 1be904b4e4..c0f5df29d2 100644
--- a/test/opt/replace_invalid_opc_test.cpp
+++ b/test/opt/replace_invalid_opc_test.cpp
@@ -404,6 +404,7 @@ TEST_F(ReplaceInvalidOpcodeTest, BarrierDontReplace) {
             OpReturn
             OpFunctionEnd)";
 
+  SetTargetEnv(SPV_ENV_UNIVERSAL_1_2);
   auto result = SinglePassRunAndDisassemble<ReplaceInvalidOpcodePass>(
       text, /* skip_nop = */ true, /* do_validation = */ false);
   EXPECT_EQ(Pass::Status::SuccessWithoutChange, std::get<1>(result));
@@ -432,9 +433,40 @@ TEST_F(ReplaceInvalidOpcodeTest, BarrierReplace) {
             OpReturn
             OpFunctionEnd)";
 
+  SetTargetEnv(SPV_ENV_UNIVERSAL_1_2);
   SinglePassRunAndMatch<ReplaceInvalidOpcodePass>(text, false);
 }
 
+// Since version 1.3 OpControlBarriers are allowed is more shaders.
+// https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpControlBarrier
+TEST_F(ReplaceInvalidOpcodeTest, BarrierDontReplaceV13) {
+  const std::string text = R"(
+            OpCapability Shader
+       %1 = OpExtInstImport "GLSL.std.450"
+            OpMemoryModel Logical GLSL450
+            OpEntryPoint Vertex %main "main"
+            OpExecutionMode %main LocalSize 1 1 1
+            OpSource GLSL 450
+            OpSourceExtension "GL_GOOGLE_cpp_style_line_directive"
+            OpSourceExtension "GL_GOOGLE_include_directive"
+            OpName %main "main"
+    %void = OpTypeVoid
+       %3 = OpTypeFunction %void
+    %uint = OpTypeInt 32 0
+  %uint_2 = OpConstant %uint 2
+%uint_264 = OpConstant %uint 264
+    %main = OpFunction %void None %3
+       %5 = OpLabel
+            OpControlBarrier %uint_2 %uint_2 %uint_264
+            OpReturn
+            OpFunctionEnd)";
+
+  SetTargetEnv(SPV_ENV_UNIVERSAL_1_3);
+  auto result = SinglePassRunAndDisassemble<ReplaceInvalidOpcodePass>(
+      text, /* skip_nop = */ true, /* do_validation = */ false);
+  EXPECT_EQ(Pass::Status::SuccessWithoutChange, std::get<1>(result));
+}
+
 TEST_F(ReplaceInvalidOpcodeTest, MessageTest) {
   const std::string text = R"(
                OpCapability Shader