Skip to content

Commit

Permalink
Do not remove control barrier after spv1.3 (KhronosGroup#5174)
Browse files Browse the repository at this point in the history 
The control barrier instruction was allowed in a limiteted set of shader types.
Part of the HLSL legalization, we use to remove the instructions when it was is
a shader in which it was not allowed. As of spv1.3 that restriction is not long
there.

This change modifies replaced invalid opc to no longer remove it.

Fixes KhronosGroup#4999.
  • Loading branch information
@s-perron
s-perron committed Mar 27, 2023
1 parent f9f31fa commit d24a39a
Show file tree
Hide file tree
Showing 4 changed files with 130 additions and 1 deletion.
11 changes: 11 additions & 0 deletions source/opt/ir_context.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
3 changes: 2 additions & 1 deletion source/opt/replace_invalid_opc.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
Expand Down
85 changes: 85 additions & 0 deletions test/opt/ir_context_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
32 changes: 32 additions & 0 deletions test/opt/replace_invalid_opc_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit d24a39a

Please sign in to comment.