Skip to content

Commit

Permalink
Switch sample_mask/index to pipeline error
Browse files Browse the repository at this point in the history 
[The spec changed](gpuweb/gpuweb#4728)
and these should now be pipeline errors

Fixed: 357047900
Bug: 357047900
Change-Id: I2b5c473e993119169db023ac534b290af2b45c20
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/201336
Commit-Queue: Gregg Tavares <[email protected]>
Reviewed-by: dan sinclair <[email protected]>
  • Loading branch information
@greggman
greggman authored and Dawn LUCI CQ committed Sep 3, 2024
1 parent 73fd374 commit f5ae33c
Show file tree
Hide file tree
Showing 6 changed files with 99 additions and 157 deletions.
10 changes: 10 additions & 0 deletions src/dawn/native/RenderPipeline.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,16 @@ ResultOrError<ShaderModuleEntryPoint> ValidateFragmentState(DeviceBase* device,
}

if (device->IsCompatibilityMode()) {
DAWN_INVALID_IF(
fragmentMetadata.usesSampleMaskOutput,
"sample_mask is not supported in compatibility mode in the fragment stage (%s, %s)",
descriptor->module, &entryPoint);

DAWN_INVALID_IF(
fragmentMetadata.usesSampleIndex,
"sample_index is not supported in compatibility mode in the fragment stage (%s, %s)",
descriptor->module, &entryPoint);

// Check that all the color target states match.
ColorAttachmentIndex firstColorTargetIndex{};
const ColorTargetState* firstColorTargetState = nullptr;
Expand Down
1 change: 1 addition & 0 deletions src/dawn/native/ShaderModule.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,7 @@ ResultOrError<std::unique_ptr<EntryPointMetadata>> ReflectEntryPointUsingTint(
totalInterStageShaderComponents += 1;
}
metadata->usesSampleMaskOutput = entryPoint.output_sample_mask_used;
metadata->usesSampleIndex = entryPoint.sample_index_used;
if (entryPoint.sample_index_used) {
totalInterStageShaderComponents += 1;
}
Expand Down
1 change: 1 addition & 0 deletions src/dawn/native/ShaderModule.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ struct EntryPointMetadata {
bool usesInstanceIndex = false;
bool usesNumWorkgroups = false;
bool usesSampleMaskOutput = false;
bool usesSampleIndex = false;
bool usesVertexIndex = false;
};

Expand Down
98 changes: 87 additions & 11 deletions src/dawn/tests/unittests/validation/CompatValidationTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,24 +213,100 @@ TEST_F(CompatValidationTest, CanNotCreatePipelineWithNonZeroDepthBiasClamp) {
ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&testDescriptor));
}

TEST_F(CompatValidationTest, CanNotUseSampleMask) {
auto wgsl = R"(
TEST_F(CompatValidationTest, CanNotUseFragmentShaderWithSampleMask) {
wgpu::ShaderModule moduleSampleMaskOutput = utils::CreateShaderModule(device, R"(
@vertex fn vs() -> @builtin(position) vec4f {
return vec4f(1);
}
struct Output {
@builtin(sample_mask) mask_out: u32,
@location(0) color : vec4f,
}
)";
ASSERT_DEVICE_ERROR(utils::CreateShaderModule(device, wgsl), //
testing::HasSubstr("sample_mask"));
@fragment fn fsWithoutSampleMaskUsage() -> @location(0) vec4f {
return vec4f(1.0, 1.0, 1.0, 1.0);
}
@fragment fn fsWithSampleMaskUsage() -> Output {
var o: Output;
// We need to make sure this sample_mask isn't optimized out even its value equals "no op".
o.mask_out = 0xFFFFFFFFu;
o.color = vec4f(1.0, 1.0, 1.0, 1.0);
return o;
}
)");

// Check we can use a fragment shader that doesn't use sample_mask from
// the same module as one that does.
{
utils::ComboRenderPipelineDescriptor descriptor;
descriptor.vertex.module = moduleSampleMaskOutput;
descriptor.cFragment.module = moduleSampleMaskOutput;
descriptor.cFragment.entryPoint = "fsWithoutSampleMaskUsage";
descriptor.multisample.count = 4;
descriptor.multisample.alphaToCoverageEnabled = false;

device.CreateRenderPipeline(&descriptor);
}

// Check we can not use a fragment shader that uses sample_mask.
{
utils::ComboRenderPipelineDescriptor descriptor;
descriptor.vertex.module = moduleSampleMaskOutput;
descriptor.cFragment.module = moduleSampleMaskOutput;
descriptor.cFragment.entryPoint = "fsWithSampleMaskUsage";
descriptor.multisample.count = 4;
descriptor.multisample.alphaToCoverageEnabled = false;

ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&descriptor),
testing::HasSubstr("sample_mask"));
}
}

TEST_F(CompatValidationTest, CanNotUseSampleIndex) {
auto wgsl = R"(
@fragment fn fsWithSampleIndexUsage(@builtin(sample_index) sNdx: u32) {
TEST_F(CompatValidationTest, CanNotUseFragmentShaderWithSampleIndex) {
wgpu::ShaderModule moduleSampleMaskOutput = utils::CreateShaderModule(device, R"(
@vertex fn vs() -> @builtin(position) vec4f {
return vec4f(1);
}
)";
ASSERT_DEVICE_ERROR(utils::CreateShaderModule(device, wgsl),
testing::HasSubstr("sample_index"));
struct Output {
@location(0) color : vec4f,
}
@fragment fn fsWithoutSampleIndexUsage() -> @location(0) vec4f {
return vec4f(1.0, 1.0, 1.0, 1.0);
}
@fragment fn fsWithSampleIndexUsage(@builtin(sample_index) sNdx: u32) -> Output {
var o: Output;
_ = sNdx;
o.color = vec4f(1.0, 1.0, 1.0, 1.0);
return o;
}
)");

// Check we can use a fragment shader that doesn't use sample_index from
// the same module as one that does.
{
utils::ComboRenderPipelineDescriptor descriptor;
descriptor.vertex.module = moduleSampleMaskOutput;
descriptor.vertex.entryPoint = "vs";
descriptor.cFragment.module = moduleSampleMaskOutput;
descriptor.cFragment.entryPoint = "fsWithoutSampleIndexUsage";
descriptor.multisample.count = 4;
descriptor.multisample.alphaToCoverageEnabled = false;

device.CreateRenderPipeline(&descriptor);
}

// Check we can not use a fragment shader that uses sample_index.
{
utils::ComboRenderPipelineDescriptor descriptor;
descriptor.vertex.module = moduleSampleMaskOutput;
descriptor.vertex.entryPoint = "vs";
descriptor.cFragment.module = moduleSampleMaskOutput;
descriptor.cFragment.entryPoint = "fsWithSampleIndexUsage";
descriptor.multisample.count = 4;
descriptor.multisample.alphaToCoverageEnabled = false;

ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&descriptor),
testing::HasSubstr("sample_index"));
}
}

TEST_F(CompatValidationTest, CanNotUseShaderWithUnsupportedInterpolateTypeOrSampling) {
Expand Down
134 changes: 0 additions & 134 deletions src/tint/lang/wgsl/resolver/compatibility_mode_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,140 +95,6 @@ INSTANTIATE_TEST_SUITE_P(ResolverCompatibilityModeTest,
core::TexelFormat::kRg32Sint,
core::TexelFormat::kRg32Uint)));

TEST_F(ResolverCompatibilityModeTest, SampleMask_Parameter) {
// @fragment
// fn main(@builtin(sample_mask) mask : u32) {
// }

Func("main",
Vector{Param(
"mask", ty.i32(),
Vector{
create<ast::BuiltinAttribute>(Source{{12, 34}}, core::BuiltinValue::kSampleMask),
})},
ty.void_(), Empty,
Vector{
Stage(ast::PipelineStage::kFragment),
},
Vector{
Builtin(core::BuiltinValue::kPosition),
});

EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(
r()->error(),
R"(12:34 error: use of '@builtin(sample_mask)' is not allowed in compatibility mode)");
}

TEST_F(ResolverCompatibilityModeTest, SampleMask_ReturnValue) {
// @fragment
// fn main() -> @builtin(sample_mask) u32 {
// return 0;
// }

Func("main", Empty, ty.u32(),
Vector{
Return(0_u),
},
Vector{
Stage(ast::PipelineStage::kFragment),
},
Vector{
create<ast::BuiltinAttribute>(Source{{12, 34}}, core::BuiltinValue::kSampleMask),
});

EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(
r()->error(),
R"(12:34 error: use of '@builtin(sample_mask)' is not allowed in compatibility mode)");
}

TEST_F(ResolverCompatibilityModeTest, SampleMask_StructMember) {
// struct S {
// @builtin(sample_mask) mask : u32,
// }

Structure("S", Vector{
Member("mask", ty.u32(),
Vector{
create<ast::BuiltinAttribute>(Source{{12, 34}},
core::BuiltinValue::kSampleMask),
}),
});

EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(
r()->error(),
R"(12:34 error: use of '@builtin(sample_mask)' is not allowed in compatibility mode)");
}

TEST_F(ResolverCompatibilityModeTest, SampleIndex_Parameter) {
// @fragment
// fn main(@builtin(sample_index) mask : u32) {
// }

Func("main",
Vector{Param(
"mask", ty.i32(),
Vector{
create<ast::BuiltinAttribute>(Source{{12, 34}}, core::BuiltinValue::kSampleIndex),
})},
ty.void_(), Empty,
Vector{
Stage(ast::PipelineStage::kFragment),
},
Vector{
Builtin(core::BuiltinValue::kPosition),
});

EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(
r()->error(),
R"(12:34 error: use of '@builtin(sample_index)' is not allowed in compatibility mode)");
}

TEST_F(ResolverCompatibilityModeTest, SampleIndex_ReturnValue) {
// @fragment
// fn main() -> @builtin(sample_index) u32 {
// return 0;
// }

Func("main", Empty, ty.u32(),
Vector{
Return(0_u),
},
Vector{
Stage(ast::PipelineStage::kFragment),
},
Vector{
create<ast::BuiltinAttribute>(Source{{12, 34}}, core::BuiltinValue::kSampleIndex),
});

EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(
r()->error(),
R"(12:34 error: use of '@builtin(sample_index)' is not allowed in compatibility mode)");
}

TEST_F(ResolverCompatibilityModeTest, SampleIndex_StructMember) {
// struct S {
// @builtin(sample_index) mask : u32,
// }

Structure("S", Vector{
Member("mask", ty.u32(),
Vector{
create<ast::BuiltinAttribute>(Source{{12, 34}},
core::BuiltinValue::kSampleIndex),
}),
});

EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(
r()->error(),
R"(12:34 error: use of '@builtin(sample_index)' is not allowed in compatibility mode)");
}

TEST_F(ResolverCompatibilityModeTest, LinearInterpolation_Parameter) {
// @fragment
// fn main(@location(1) @interpolate(linear) value : f32) {
Expand Down
12 changes: 0 additions & 12 deletions src/tint/lang/wgsl/resolver/validator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1067,12 +1067,6 @@ bool Validator::BuiltinAttribute(const ast::BuiltinAttribute* attr,
}
break;
case core::BuiltinValue::kSampleMask:
if (mode_ == wgsl::ValidationMode::kCompat) {
AddError(attr->source) << "use of " << style::Attribute("@builtin")
<< style::Code("(", style::Enum(builtin), ")")
<< " is not allowed in compatibility mode";
return false;
}
if (stage != ast::PipelineStage::kNone && !(stage == ast::PipelineStage::kFragment)) {
is_stage_mismatch = true;
}
Expand All @@ -1082,12 +1076,6 @@ bool Validator::BuiltinAttribute(const ast::BuiltinAttribute* attr,
}
break;
case core::BuiltinValue::kSampleIndex:
if (mode_ == wgsl::ValidationMode::kCompat) {
AddError(attr->source) << "use of " << style::Attribute("@builtin")
<< style::Code("(", style::Enum(builtin), ")")
<< " is not allowed in compatibility mode";
return false;
}
if (stage != ast::PipelineStage::kNone &&
!(stage == ast::PipelineStage::kFragment && is_input)) {
is_stage_mismatch = true;
Expand Down

0 comments on commit f5ae33c

Please sign in to comment.