Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validation for relaxed control barrier with storage class semantics #5984

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+54 −14
Open

Validation for relaxed control barrier with storage class semantics #5984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions source/val/validate_memory_semantics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,7 @@ spv_result_t ValidateMemorySemantics(ValidationState_t& _,
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< spvOpcodeString(opcode)
<< ": Memory Semantics can have at most one of the following "
"bits "
"set: Acquire, Release, AcquireRelease or "
"bits set: Acquire, Release, AcquireRelease or "
"SequentiallyConsistent";
}

Expand Down Expand Up @@ -176,10 +175,8 @@ spv_result_t ValidateMemorySemantics(ValidationState_t& _,
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< _.VkErrorID(4732) << spvOpcodeString(opcode)
<< ": Vulkan specification requires Memory Semantics to have "
"one "
"of the following bits set: Acquire, Release, "
"AcquireRelease "
"or SequentiallyConsistent";
"one of the following bits set: Acquire, Release, "
"AcquireRelease or SequentiallyConsistent";
} else if (opcode != spv::Op::OpMemoryBarrier &&
num_memory_order_set_bits) {
// should leave only atomics and control barriers for Vulkan env
Expand All @@ -203,11 +200,20 @@ spv_result_t ValidateMemorySemantics(ValidationState_t& _,
"storage class";
}

if (opcode == spv::Op::OpControlBarrier && value && !includes_storage_class) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< _.VkErrorID(4650) << spvOpcodeString(opcode)
<< ": expected Memory Semantics to include a Vulkan-supported "
"storage class if Memory Semantics is not None";
if (opcode == spv::Op::OpControlBarrier && value) {
if (!num_memory_order_set_bits) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< _.VkErrorID(4649) << spvOpcodeString(opcode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This VUID does not exist. There needs to be a Vulkan-Docs PR to add this rule, at which point a VUID will be assigned.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created this one KhronosGroup/Vulkan-Docs#2495

Copy link
Contributor

@spencer-lunarg spencer-lunarg Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might take awhile (like 2 or 3 weeks) to get the VUID in and upstreamed, I am happy to come back here and apply it, but for the short term, just remove VkErrorID() here, and the 4649 stuff everywhere else

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think waiting 2–3 weeks should be fine. The main change in this PR is adding a new validation rule, and without the new VUID, there wouldn’t be much left.

<< ": Vulkan specification requires non-zero Memory Semantics "
"to have one of the following bits set: Acquire, Release, "
"AcquireRelease or SequentiallyConsistent";
}
if (!includes_storage_class) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< _.VkErrorID(4650) << spvOpcodeString(opcode)
<< ": expected Memory Semantics to include a Vulkan-supported "
"storage class if Memory Semantics is not None";
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions source/val/validation_state.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2311,6 +2311,8 @@ std::string ValidationState_t::VkErrorID(uint32_t id,
return VUID_WRAP(VUID-StandaloneSpirv-None-04644);
case 4645:
return VUID_WRAP(VUID-StandaloneSpirv-None-04645);
case 4649:
return VUID_WRAP(VUID-StandaloneSpirv-OpControlBarrier-04649);
case 4650:
return VUID_WRAP(VUID-StandaloneSpirv-OpControlBarrier-04650);
case 4651:
Expand Down
38 changes: 35 additions & 3 deletions test/val/val_barriers_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,39 @@ OpControlBarrier %workgroup %device %acquire_release_subgroup
"Vulkan-supported storage class if Memory Semantics is not None"));
}

TEST_F(ValidateBarriers, OpControlBarrierVulkanAcquireRelease) {
const std::string body = R"(
OpControlBarrier %workgroup %device %acquire_release
)";

CompileSuccessfully(GenerateShaderCode(body), SPV_ENV_VULKAN_1_0);
ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
EXPECT_THAT(getDiagnosticString(),
AnyVUID("VUID-StandaloneSpirv-OpControlBarrier-04650"));
EXPECT_THAT(
getDiagnosticString(),
HasSubstr(
"ControlBarrier: expected Memory Semantics to include a "
"Vulkan-supported storage class if Memory Semantics is not None"));
}

TEST_F(ValidateBarriers, OpControlBarrierVulkanWorkgroupMemory) {
const std::string body = R"(
OpControlBarrier %workgroup %workgroup %workgroup_memory
)";

CompileSuccessfully(GenerateShaderCode(body), SPV_ENV_VULKAN_1_0);
ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(SPV_ENV_VULKAN_1_0));
EXPECT_THAT(getDiagnosticString(),
AnyVUID("VUID-StandaloneSpirv-OpControlBarrier-04649"));
EXPECT_THAT(
getDiagnosticString(),
HasSubstr(
"ControlBarrier: Vulkan specification requires non-zero "
"Memory Semantics to have one of the following bits set: Acquire, "
"Release, AcquireRelease or SequentiallyConsistent"));
}

TEST_F(ValidateBarriers, OpControlBarrierSubgroupExecutionFragment1p1) {
const std::string body = R"(
OpControlBarrier %subgroup %subgroup %acquire_release_workgroup
Expand Down Expand Up @@ -687,9 +720,8 @@ OpControlBarrier %subgroup %workgroup %acquire_release_workgroup
ASSERT_EQ(SPV_ERROR_INVALID_ID, ValidateInstructions(SPV_ENV_VULKAN_1_0));
EXPECT_THAT(getDiagnosticString(),
HasSubstr("OpControlBarrier requires one of the following "
"Execution "
"Models: TessellationControl, GLCompute, Kernel, "
"MeshNV or TaskNV"));
"Execution Models: TessellationControl, GLCompute, "
"Kernel, MeshNV or TaskNV"));
}

TEST_F(ValidateBarriers, OpMemoryBarrierSuccess) {
Expand Down