Skip to content

Commit

Permalink
[naga] Validate that all order-sensitive expressions are emitted.
Browse files Browse the repository at this point in the history
Ensure that every expression whose value is affected by statements'
side effects is covered by an `Emit` statement.

See the comments on `Expression::is_order_sensitive` for details.

Fixes gfx-rs#5763.
  • Loading branch information
jimblandy committed Jun 2, 2024
1 parent ef0d93a commit fd3a81d
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 2 deletions.
21 changes: 21 additions & 0 deletions naga/src/proc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,27 @@ impl crate::Expression {
_ => true,
}
}

/// Return true if this expression's value is sensitive to when it is evaluated.
///
/// Expressions like [`Load`] and [`ImageLoad`] can produce different
/// values, even when their operands' values are unchanged, because they are
/// sensitive to side effects of other statements in the function.
///
/// Such order-sensitive expressions must be covered by an [`Emit`]
/// statement, ensuring that front ends have fully specified the ordering
/// the input language requires, or chosen one arbitrarily if the input
/// language doesn't care.
///
/// [`Load`]: crate::Expression::Load
/// [`ImageLoad`]: crate::Expression::ImageLoad
/// [`Emit`]: crate::Statement::Emit
pub const fn is_order_sensitive(&self) -> bool {
match *self {
Self::Load { .. } | Self::ImageLoad { .. } => true,
_ => false,
}
}
}

impl crate::Function {
Expand Down
22 changes: 20 additions & 2 deletions naga/src/valid/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,8 @@ pub enum FunctionError {
InvalidSubgroup(#[from] SubgroupError),
#[error("Emit statement should not cover \"result\" expressions like {0:?}")]
EmitResult(Handle<crate::Expression>),
#[error("Order-sensitive expression not covered by an `Emit` statement: {0:?}")]
EmitMissing(Handle<crate::Expression>),
}

bitflags::bitflags! {
Expand Down Expand Up @@ -570,9 +572,7 @@ impl super::Validator {
| Ex::FunctionArgument(_)
| Ex::GlobalVariable(_)
| Ex::LocalVariable(_)
| Ex::Load { .. }
| Ex::ImageSample { .. }
| Ex::ImageLoad { .. }
| Ex::ImageQuery { .. }
| Ex::Unary { .. }
| Ex::Binary { .. }
Expand All @@ -585,6 +585,10 @@ impl super::Validator {
| Ex::RayQueryGetIntersection { .. } => {
self.emit_expression(handle, context)?
}
Ex::Load { .. } | Ex::ImageLoad { .. } => {
self.order_sensitive_expression_set.remove(handle.index());
self.emit_expression(handle, context)?
}
Ex::CallResult(_)
| Ex::AtomicResult { .. }
| Ex::WorkGroupUniformLoadResult { .. }
Expand Down Expand Up @@ -1307,11 +1311,16 @@ impl super::Validator {

self.valid_expression_set.clear();
self.valid_expression_list.clear();
self.order_sensitive_expression_set.clear();
for (handle, expr) in fun.expressions.iter() {
if expr.needs_pre_emit() {
self.valid_expression_set.insert(handle.index());
}
if self.flags.contains(super::ValidationFlags::EXPRESSIONS) {
if expr.is_order_sensitive() {
self.order_sensitive_expression_set.insert(handle.index());
}

match self.validate_expression(
handle,
expr,
Expand All @@ -1338,6 +1347,15 @@ impl super::Validator {
)?
.stages;
info.available_stages &= stages;

if self.flags.contains(super::ValidationFlags::EXPRESSIONS) {
if let Some(expr_index) = self.order_sensitive_expression_set.iter().next() {
let expr_index = std::num::NonZeroU32::new((expr_index + 1) as u32).unwrap();
let expr_handle = Handle::new(expr_index);
return Err(FunctionError::EmitMissing(expr_handle)
.with_span_handle(expr_handle, &fun.expressions));
}
}
}
Ok(info)
}
Expand Down
10 changes: 10 additions & 0 deletions naga/src/valid/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,15 @@ pub struct Validator {
switch_values: FastHashSet<crate::SwitchValue>,
valid_expression_list: Vec<Handle<crate::Expression>>,
valid_expression_set: BitSet,

/// The set of expressions that must be covered by an [`Emit`].
///
/// [`Expression::is_order_sensitive`] returns `true` for expressions that
/// must be covered by an [`Emit`] statement to ensure they produce the
/// value expected by the front end. We add such expressions to this set
/// during expression validation, and "check them off" (that is, remove) them
/// during block validation when we see an [`Emit`] statement that covers them.
order_sensitive_expression_set: BitSet,
override_ids: FastHashSet<u16>,
allow_overrides: bool,
}
Expand Down Expand Up @@ -383,6 +392,7 @@ impl Validator {
switch_values: FastHashSet::default(),
valid_expression_list: Vec::new(),
valid_expression_set: BitSet::new(),
order_sensitive_expression_set: BitSet::new(),
override_ids: FastHashSet::default(),
allow_overrides: true,
}
Expand Down

0 comments on commit fd3a81d

Please sign in to comment.