Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Naga doesn't check that subexpressions have been emitted. #5763

Open
jimblandy opened this issue Jun 1, 2024 · 2 comments · May be fixed by #5769
Open

Naga doesn't check that subexpressions have been emitted. #5763

jimblandy opened this issue Jun 1, 2024 · 2 comments · May be fixed by #5769
Labels
area: validation Issues related to validation, diagnostics, and error handling naga Shader Translator type: bug Something isn't working

Comments

@jimblandy
Copy link
Member

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 jimblandy added type: bug Something isn't working area: validation Issues related to validation, diagnostics, and error handling naga Shader Translator labels Jun 1, 2024
@jimblandy
Copy link
Member Author

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.

jimblandy added a commit to jimblandy/wgpu that referenced this issue Jun 2, 2024
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.
@teoxoy
Copy link
Member

teoxoy commented Jun 3, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: validation Issues related to validation, diagnostics, and error handling naga Shader Translator type: bug Something isn't working
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants