Naga doesn't check that subexpressions have been emitted.

[jimblandy]

Naga doesn't check that each subexpression of an expression used by a statement has been covered by an Emit statement.

When added to naga/tests/validation.rs, the following test should pass, but it fails:

/// 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);
        let ex_42 = fun.expressions.append(Expression::Literal(naga::Literal::U32(42)), 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::Binary {
            op: naga::BinaryOperator::Add,
            left: ex_42,
            right: ex_42,
        }, 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);

        valid::Validator::new(
            valid::ValidationFlags::default(),
            valid::Capabilities::all(),
        )
        .validate(&module)
    }

    variant(true).expect("module should validate");
    variant(false).expect_err("validation should notice un-emitted subexpression");
}

[jimblandy]

It's not clear that this must be a problem. We could simply decide that Naga's validation rules permit subexpressions that are not covered by Emit statements, and backends are free to evaluate such subexpressions whenever is convenient. It seems that our backends work this way already, by accident.

Here's an illustration of the impact. Emit statements determine when Load expressions are evaluated relative to side effects that could affect the values they produce. For example:

var<private> x: i32 = 0;

fn inc_x() -> i32 {
    x++;
    return x;
}

fn f() -> i32 {
    x = 0;
    return x + inc_x();
}

In x + inc_x(), WGSL requires the subexpression x to be evaluated before the call to inc_x takes place, per the rules in §14.1. Program Order Within an Invocation. So the WGSL standard is clear that calling f must return 1: the value of the subexpression x is 0, and inc_x() returns 1.

To get this behavior, however, Naga's WGSL front end must ensure that, in the Module it generates, the Load of x in f is covered by an Emit statement before the Call statement that invokes inc_x. Otherwise, the Load will not be evaluated until the Return statement, as if the source code had been:

return inc_x() + x;

and the program will return 2.

The reason this issue isn't necessarily a bug in Naga's evaluator is that, if the Load is not covered by an Emit, that is arguably a bug in Naga's WGSL front end: it did not properly translate the meaning of the WGSL source code into Naga IR.

However, I think this is unsatisfying: if a front end neglects to specify when an order-sensitive expression like Load is evaluated, rather than saying "do as you wish, backends!" we should help front-end developers by rejecting the module, and making them spell out the evaluation order they want.

[teoxoy]

I'm not sure what the intention was when we initially implemented the emitting machinery.

However, I think this is unsatisfying: if a front end neglects to specify when an order-sensitive expression like Load is evaluated, rather than saying "do as you wish, backends!" we should help front-end developers by rejecting the module, and making them spell out the evaluation order they want.

I think this is a good idea, we'd avoid a possible footgun.

Ideally we'd remove the emitting concept altogether. I really liked the Instruction concept we previously talked about.