[naga] Validate that all order-sensitive expressions are emitted.

naga/src/proc/mod.rs

@@ -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 {

naga/src/valid/function.rs

@@ -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! {
@@ -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 { .. }
@@ -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 { .. }
@@ -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,
@@ -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)
     }

naga/src/valid/mod.rs

@@ -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,
 }
@@ -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,
         }

naga/tests/validation.rs

@@ -1,9 +1,7 @@
-use naga::{valid, Expression, Function, Scalar};
+use naga::{valid, Expression, Function, Module, Scalar, Type, TypeInner};
 
 #[test]
 fn emit_atomic_result() {
-    use naga::{Module, Type, TypeInner};
-
     // We want to ensure that the *only* problem with the code is the
     // use of an `Emit` statement instead of an `Atomic` statement. So
     // validate two versions of the module varying only in that
@@ -88,8 +86,6 @@ fn emit_atomic_result() {
 
 #[test]
 fn emit_call_result() {
-    use naga::{Module, Type, TypeInner};
-
     // We want to ensure that the *only* problem with the code is the
     // use of an `Emit` statement instead of a `Call` statement. So
     // validate two versions of the module varying only in that
@@ -161,8 +157,6 @@ fn emit_call_result() {
 
 #[test]
 fn emit_workgroup_uniform_load_result() {
-    use naga::{Module, Type, TypeInner};
-
     // We want to ensure that the *only* problem with the code is the
     // use of an `Emit` statement instead of an `Atomic` statement. So
     // validate two versions of the module varying only in that
@@ -228,3 +222,100 @@ fn emit_workgroup_uniform_load_result() {
     variant(true).expect("module should validate");
     assert!(variant(false).is_err());
 }
+
+/// Validation should reject expressions that refer to un-emitted
+/// subexpressions.
+#[test]
+fn emit_subexpressions() {
+    fn variant(
+        emit: bool,
+    ) -> Result<naga::valid::ModuleInfo, naga::WithSpan<naga::valid::ValidationError>> {
+        let span = naga::Span::default();
+        let mut module = Module::default();
+        let ty_u32 = module.types.insert(
+            Type {
+                name: Some("u32".into()),
+                inner: TypeInner::Scalar(Scalar::U32),
+            },
+            span,
+        );
+        let var_private = module.global_variables.append(
+            naga::GlobalVariable {
+                name: Some("private".into()),
+                space: naga::AddressSpace::Private,
+                binding: None,
+                ty: ty_u32,
+                init: None,
+            },
+            span,
+        );
+
+        let mut fun = Function::default();
+
+        // These expressions are pre-emit, so they don't need to be
+        // covered by any `Emit` statement.
+        let ex_var = fun
+            .expressions
+            .append(Expression::GlobalVariable(var_private), span);
+
+        // This expression is neither pre-emit nor used directly by a
+        // statement. We want to test whether validation notices when
+        // it is not covered by an `Emit` statement.
+        let ex_add = fun
+            .expressions
+            .append(Expression::Load { pointer: ex_var }, span);
+
+        // This expression is used directly by the statement, so if
+        // it's not covered by an `Emit`, then validation will catch
+        // that.
+        let ex_mul = fun.expressions.append(
+            Expression::Binary {
+                op: naga::BinaryOperator::Multiply,
+                left: ex_add,
+                right: ex_add,
+            },
+            span,
+        );
+
+        if emit {
+            // This `Emit` covers all expressions properly.
+            fun.body.push(
+                naga::Statement::Emit(naga::Range::new_from_bounds(ex_add, ex_mul)),
+                span,
+            );
+        } else {
+            // This `Emit` covers `ex_mul` but not its subexpression `ex_add`.
+            fun.body.push(
+                naga::Statement::Emit(naga::Range::new_from_bounds(ex_mul, ex_mul)),
+                span,
+            );
+        }
+        fun.body.push(
+            naga::Statement::Store {
+                pointer: ex_var,
+                value: ex_mul,
+            },
+            span,
+        );
+
+        module.functions.append(fun, span);
+
+        let result = valid::Validator::new(
+            valid::ValidationFlags::default(),
+            valid::Capabilities::all(),
+        )
+        .validate(&module);
+
+        if let Ok(ref info) = result {
+            let (source, _translation_info) =
+                naga::back::msl::write_string(&module, info, &<_>::default(), &<_>::default())
+                    .expect("generating MSL failed");
+            eprintln!("MSL output:\n{source}");
+        }
+
+        result
+    }
+
+    variant(true).expect("module should validate");
+    variant(false).expect_err("validation should notice un-emitted subexpression");
+}