Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,10 @@ By @kpreid in [#9042](https://github.com/gfx-rs/wgpu/pull/9042).
- Tracing support has been restored. By @andyleiserson in [#8429](https://github.com/gfx-rs/wgpu/pull/8429).
- Pipelines using passthrough shaders now correctly require explicit pipeline layout. By @inner-daemons in #8881.
- Allow using a shader that defines I/O for dual-source blending in a pipeline that does not make use of it. By @andyleiserson in [#8856](https://github.com/gfx-rs/wgpu/pull/8856).
- Improve validation of dual-source blending, by @andyleiserson in [#9200](https://github.com/gfx-rs/wgpu/pull/9200):
- Validate structs with `@blend_src` members whether or not they are used by an entry point.
Copy link
Member

Choose a reason for hiding this comment

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

thought: This is/these are technically a breaking change, which we normally advertise, but this is a bug fix for spec. validation. My other validation fix PRs have advertised these as BREAKING, so maybe we should talk about some consistent criteria for doing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a borderline case. Somewhat recently I added a "validation" section in the changelog which lists a bunch of validation changes, at least some of which probably make it stricter, and none of which are noted as breaking.

That seems right to me... I think it's worth reserving the "breaking" label for API changes that require users to update their code, or for validation changes that we think carry a high risk of breakage. Breaking code that wouldn't work in Chrome/Dawn (although I can't say I specifically checked that here), that was never legal under the spec, and where the problem will be obvious (shader compilation error rather than e.g. a runtime behavior change), doesn't seem like it needs to be called out at the same level and dilute the communication of the truly breaking changes.

- Dual-source blending is not supported when there are multiple color attachments.
- `TypeFlags::IO_SHAREABLE` is not set for structs other than `@blend_src` structs.
- Validate `strip_index_format` isn't None and equals index buffer format for indexed drawing with strip topology. By @beicause in [#8850](https://github.com/gfx-rs/wgpu/pull/8850).
- Renamed `EXPERIMENTAL_PASSTHROUGH_SHADERS` to `PASSTHROUGH_SHADERS` and made this no longer an experimental feature. by @inner-daemons in [#9054](https://github.com/gfx-rs/wgpu/pull/9054).
- BREAKING: End offsets in trace and `player` commands are now represented using `offset` + `size` instead. By @ErichDonGubler in [9073](https://github.com/gfx-rs/wgpu/pull/9073).
Expand Down
7 changes: 2 additions & 5 deletions cts_runner/test.lst
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ webgpu:api,validation,render_pass,resolve:resolve_attachment:*
webgpu:api,validation,render_pipeline,depth_stencil_state:*
webgpu:api,validation,render_pipeline,float32_blendable:*
webgpu:api,validation,render_pipeline,fragment_state:color_target_exists:*
webgpu:api,validation,render_pipeline,fragment_state:dual_source_blending,use_blend_src:*
webgpu:api,validation,render_pipeline,fragment_state:dual_source_blending,*
webgpu:api,validation,render_pipeline,fragment_state:limits,*
webgpu:api,validation,render_pipeline,fragment_state:targets_blend:*
webgpu:api,validation,render_pipeline,fragment_state:targets_format_filterable:*
Expand Down Expand Up @@ -415,10 +415,7 @@ webgpu:shader,validation,expression,call,builtin,value_constructor:*
webgpu:shader,validation,expression,call,builtin,workgroupUniformLoad:*
webgpu:shader,validation,expression,overload_resolution:*
webgpu:shader,validation,extension,clip_distances:*
webgpu:shader,validation,extension,dual_source_blending:blend_src_same_type:*
webgpu:shader,validation,extension,dual_source_blending:blend_src_stage_input_output:*
webgpu:shader,validation,extension,dual_source_blending:blend_src_syntax_validation:*
webgpu:shader,validation,extension,dual_source_blending:use_blend_src_requires_extension_enabled:*
webgpu:shader,validation,extension,dual_source_blending:*
webgpu:shader,validation,extension,pointer_composite_access:*
webgpu:shader,validation,functions,restrictions:param_type_can_be_alias:*
webgpu:shader,validation,parse,blankspace:blankspace:*
Expand Down
134 changes: 63 additions & 71 deletions naga/src/valid/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@ pub enum GlobalVariableError {
pub enum VaryingError {
#[error("The type {0:?} does not match the varying")]
InvalidType(Handle<crate::Type>),
#[error("The type {0:?} cannot be used for user-defined entry point inputs or outputs")]
#[error(
"The type {0:?} cannot be used for user-defined entry point inputs or outputs. \
Only numeric scalars and vectors are allowed."
)]
NotIOShareableType(Handle<crate::Type>),
#[error("Interpolation is not valid")]
InvalidInterpolation,
Expand Down Expand Up @@ -93,15 +96,23 @@ pub enum VaryingError {
InvalidInputAttributeInStage(&'static str, crate::ShaderStage),
#[error("The attribute {0:?} is not valid for stage {1:?}")]
InvalidAttributeInStage(&'static str, crate::ShaderStage),
#[error("The `blend_src` attribute can only be used on location 0, only indices 0 and 1 are valid. Location was {location}, index was {blend_src}.")]
#[error("`@blend_src` can only be used at location 0, indices 0 and 1. Found `@location({location}) @blend_src({blend_src})`.")]
InvalidBlendSrcIndex { location: u32, blend_src: u32 },
#[error("If `blend_src` is used, there must be exactly two outputs both with location 0, one with `blend_src(0)` and the other with `blend_src(1)`.")]
IncompleteBlendSrcUsage,
#[error("If `blend_src` is used, both outputs must have the same type. `blend_src(0)` has type {blend_src_0_type:?} and `blend_src(1)` has type {blend_src_1_type:?}.")]
#[error(
"`@blend_src` structure must specify two sources. \
Found `@blend_src({present_blend_src})` but not `@blend_src({absent_blend_src})`.",
absent_blend_src = if *present_blend_src == 0 { 1 } else { 0 },
)]
IncompleteBlendSrcUsage { present_blend_src: u32 },
#[error("Structure using `@blend_src` may not specify `@location` on any other members. Found a binding at `@location({location})`.")]
InvalidBlendSrcWithOtherBindings { location: u32 },
#[error("Both `@blend_src` structure members must have the same type. `blend_src(0)` has type {blend_src_0_type:?} and `blend_src(1)` has type {blend_src_1_type:?}.")]
BlendSrcOutputTypeMismatch {
blend_src_0_type: Handle<crate::Type>,
blend_src_1_type: Handle<crate::Type>,
},
#[error("`@blend_src` can only be used on struct members, not directly on entry point I/O")]
BlendSrcNotOnStructMember,
#[error("Workgroup size is multi dimensional, `@builtin(subgroup_id)` and `@builtin(subgroup_invocation_id)` are not supported.")]
InvalidMultiDimensionalSubgroupBuiltIn,
#[error("The `@per_primitive` attribute can only be used in fragment shader inputs or mesh shader primitive outputs")]
Expand Down Expand Up @@ -209,7 +220,7 @@ struct VaryingContext<'a> {
types: &'a UniqueArena<crate::Type>,
type_info: &'a Vec<super::r#type::TypeInfo>,
location_mask: &'a mut BitSet,
blend_src_mask: &'a mut BitSet,
dual_source_blending: Option<&'a mut bool>,
built_ins: &'a mut crate::FastHashSet<crate::BuiltIn>,
capabilities: Capabilities,
flags: super::ValidationFlags,
Expand Down Expand Up @@ -721,38 +732,8 @@ impl VaryingContext<'_> {
return Err(VaryingError::InvalidPerPrimitive);
}

if let Some(blend_src) = blend_src {
// `blend_src` is only valid if dual source blending was explicitly enabled,
// see https://www.w3.org/TR/WGSL/#extension-dual_source_blending
if !self
.capabilities
.contains(Capabilities::DUAL_SOURCE_BLENDING)
{
return Err(VaryingError::UnsupportedCapability(
Capabilities::DUAL_SOURCE_BLENDING,
));
}
if self.stage != crate::ShaderStage::Fragment {
return Err(VaryingError::InvalidAttributeInStage(
"blend_src",
self.stage,
));
}
if !self.output {
return Err(VaryingError::InvalidInputAttributeInStage(
"blend_src",
self.stage,
));
}
if (blend_src != 0 && blend_src != 1) || location != 0 {
return Err(VaryingError::InvalidBlendSrcIndex {
location,
blend_src,
});
}
if !self.blend_src_mask.insert(blend_src as usize) {
return Err(VaryingError::BindingCollisionBlendSrc { blend_src });
}
if blend_src.is_some() {
return Err(VaryingError::BlendSrcNotOnStructMember);
} else if !self.location_mask.insert(location as usize)
&& self.flags.contains(super::ValidationFlags::BINDINGS)
{
Expand Down Expand Up @@ -852,37 +833,51 @@ impl VaryingContext<'_> {
}
};

for (index, member) in members.iter().enumerate() {
let span_context = self.types.get_span_context(ty);
match member.binding {
None => {
if self.flags.contains(super::ValidationFlags::BINDINGS) {
return Err(VaryingError::MemberMissingBinding(index as u32)
.with_span_context(span_context));
}
}
Some(ref binding) => self
.validate_impl(ep, member.ty, binding)
.map_err(|e| e.with_span_context(span_context))?,
}
}

if !self.blend_src_mask.is_empty() {
let span_context = self.types.get_span_context(ty);

// If there's any blend_src usage, it must apply to all members of which there must be exactly 2.
if members.len() != 2 || self.blend_src_mask.count() != 2 {
if self.type_info[ty.index()]
.flags
.contains(super::TypeFlags::IO_SHAREABLE)
{
// `@blend_src` is the only case where `IO_SHAREABLE` is set on a struct (as
// opposed to members of a struct). The struct definition is validated during
// type validation.
if self.stage != crate::ShaderStage::Fragment {
return Err(
VaryingError::IncompleteBlendSrcUsage.with_span_context(span_context)
VaryingError::InvalidAttributeInStage("blend_src", self.stage)
.with_span(),
);
}
// Also, all members must have the same type.
if members[0].ty != members[1].ty {
return Err(VaryingError::BlendSrcOutputTypeMismatch {
blend_src_0_type: members[0].ty,
blend_src_1_type: members[1].ty,
if !self.output {
return Err(VaryingError::InvalidInputAttributeInStage(
"blend_src",
self.stage,
)
.with_span());
}
// Dual blend sources must always be at location 0.
if !self.location_mask.insert(0)
&& self.flags.contains(super::ValidationFlags::BINDINGS)
{
return Err(VaryingError::BindingCollision { location: 0 }.with_span());
}

**self
.dual_source_blending
.as_mut()
.expect("unexpected dual source blending") = true;
} else {
for (index, member) in members.iter().enumerate() {
let span_context = self.types.get_span_context(ty);
match member.binding {
None => {
if self.flags.contains(super::ValidationFlags::BINDINGS) {
return Err(VaryingError::MemberMissingBinding(index as u32)
.with_span_context(span_context));
}
}
Some(ref binding) => self
.validate_impl(ep, member.ty, binding)
.map_err(|e| e.with_span_context(span_context))?,
}
.with_span_context(span_context));
}
}
Ok(())
Expand Down Expand Up @@ -1198,7 +1193,7 @@ impl super::Validator {
types: &module.types,
type_info: &self.types,
location_mask: &mut self.location_mask,
blend_src_mask: &mut self.blend_src_mask,
dual_source_blending: None,
built_ins: &mut result_built_ins,
capabilities: self.capabilities,
flags: self.flags,
Expand Down Expand Up @@ -1370,7 +1365,7 @@ impl super::Validator {
types: &module.types,
type_info: &self.types,
location_mask: &mut self.location_mask,
blend_src_mask: &mut self.blend_src_mask,
dual_source_blending: Some(&mut info.dual_source_blending),
built_ins: &mut argument_built_ins,
capabilities: self.capabilities,
flags: self.flags,
Expand All @@ -1390,7 +1385,7 @@ impl super::Validator {
types: &module.types,
type_info: &self.types,
location_mask: &mut self.location_mask,
blend_src_mask: &mut self.blend_src_mask,
dual_source_blending: Some(&mut info.dual_source_blending),
built_ins: &mut result_built_ins,
capabilities: self.capabilities,
flags: self.flags,
Expand Down Expand Up @@ -1418,9 +1413,6 @@ impl super::Validator {
return Err(EntryPointError::WrongTaskShaderEntryResult.with_span());
}
}
if !self.blend_src_mask.is_empty() {
info.dual_source_blending = true;
}
} else if ep.stage == crate::ShaderStage::Vertex {
return Err(EntryPointError::MissingVertexOutputPosition.with_span());
} else if ep.stage == crate::ShaderStage::Task {
Expand Down
3 changes: 0 additions & 3 deletions naga/src/valid/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,6 @@ pub struct Validator {
types: Vec<r#type::TypeInfo>,
layouter: Layouter,
location_mask: BitSet,
blend_src_mask: BitSet,
ep_resource_bindings: FastHashSet<crate::ResourceBinding>,
switch_values: FastHashSet<crate::SwitchValue>,
valid_expression_list: Vec<Handle<crate::Expression>>,
Expand Down Expand Up @@ -608,7 +607,6 @@ impl Validator {
types: Vec::new(),
layouter: Layouter::default(),
location_mask: BitSet::new(),
blend_src_mask: BitSet::new(),
ep_resource_bindings: FastHashSet::default(),
switch_values: FastHashSet::default(),
valid_expression_list: Vec::new(),
Expand Down Expand Up @@ -638,7 +636,6 @@ impl Validator {
self.types.clear();
self.layouter.clear();
self.location_mask.make_empty();
self.blend_src_mask.make_empty();
self.ep_resource_bindings.clear();
self.switch_values.clear();
self.valid_expression_list.clear();
Expand Down
Loading
Loading