Implement const-expressions (phase 1)

[teoxoy]

First phase of gfx-rs/wgpu#4484.

• fixes [wgsl-in] Rejection of matrix consts #1956
• fixes [wgsl-in] Rejection of composite consts without explicit nested types #1957
• fixes [wgsl-in] Missing integer type validation for contants #2311

[jimblandy]

Some comments for now. Once you've done the Specialization rename, then I have some doc comments to push to the branch.

[jimblandy]

I find myself wishing that Module::const_expressions was override_expressions, and that all constant expressions got evaluated in the front end. But this would mean:

• Named constants and such would not be preserved in the output - they'd be replaced with their evaluated values.
• We'd need to retain something like ConstantInner to represent constants of composite types.

For the latter point, I suspect that supporting override expressions is going to make us introduce something like that anyway.

[jimblandy]

WGSL const-expressions may have abstract type. We should not introduce abstract types into the IR, so that means that, at least sometimes we will not be able to render WGSL const declarations as Constants in the IR.

[teoxoy]

WGSL const-expressions may have abstract type. We should not introduce abstract types into the IR, so that means that, at least sometimes we will not be able to render WGSL const declarations as Constants in the IR.

I think we will have to have abstract types at the IR level because any expression can potentially be a const-expression (not only those in const_expressions).

[jimblandy]

I haven't debugged this, but it looks like the problems the WGSL front end rewrite used to have when it forgot to switch arenas:

var<private> arr: array<i32, 10> = array<i32,10>(0, 0, 0, 0, 0, 0, 0, 0, 0, 0);

const const_zero: i32 = 0;

fn test_arr_as_arg(a: array<f32, 10>) -> f32 {
    return a[const_zero];
}

This gives me:

thread 'main' panicked at 'index out of bounds: the len is 3 but the index is 13', src/proc/mod.rs:594:19

Bizarrely, RUST_BACKTRACE=1 does not give me a full stack - the only interesting frames are:

   6: <naga::arena::Arena<T> as core::ops::index::Index<naga::arena::Handle<T>>>::index
             at /home/jimb/rust/naga/src/arena.rs:413:10
   7: naga::proc::GlobalCtx::eval_expr_to_u32::get
             at /home/jimb/rust/naga/src/proc/mod.rs:594:19

[jimblandy]

I've split out the bottom commit, 71435ba ("revert af4d989 since d21dded removes the need for it"), into #2331 and landed that.

[jimblandy]

Further, I suspect that when we fix the bad handle we'll find that when the WGSL front end lowers an ast::Expression::Index expression's index operant to a crate::Expression::Constant expression, it'll generate a crate::Expression::Access expression, not an AccessIndex - and the test case will still fail to work because we don't implement Access on by-value arrays yet.

[jimblandy]

I've split out the introduction of Expression::ZeroValue into #2332.

[jimblandy]

I think I've got Expression::Literal split out as well, but there are a few tangles.

[teoxoy]

I pushed a commit that fixes #2266 (review), it looks like I already fixed it in the phase 2 PR as well.

[jimblandy]

When I run cargo nextest run -p naga, I get the following warnings:

warning: unused import: `std::ops::Index`
 --> src/valid/expression.rs:1:5
  |
1 | use std::ops::Index;
  |     ^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: unused imports: `ResolveContext`, `U32EvalError`
  --> src/valid/expression.rs:13:34
   |
13 |     proc::{IndexableLengthError, ResolveContext, ResolveError, U32EvalError},
   |                                  ^^^^^^^^^^^^^^                ^^^^^^^^^^^^

warning: unused import: `Arena`
  --> src/valid/mod.rs:19:5
   |
19 |     Arena, FastHashSet,
   |     ^^^^^

warning: method `eval_expr_to_u32` is never used
   --> src/proc/mod.rs:573:19
    |
571 | impl GlobalCtx<'_> {
    | ------------------ method in this implementation
572 |     /// Try to evaluate the expr...
573 |     pub(crate) fn eval_expr_to_u32(
    |                   ^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

The first one is present on master as well, and is fixed by #2383, but the others I think are introduced on this branch.

[teoxoy]

We should cargo check naga in CI without any feature flags, I think right now we only check with all-features.

[jimblandy]

@teoxoy If I add this test to src/valid/handles.rs, it fails, but I think it should pass:

#[test]
#[cfg(feature = "validate")]
fn constant_deps() {
    use crate::{Constant, Expression, Span, Type, TypeInner};

    let nowhere = Span::default();
    
    let mut types = UniqueArena::new();
    let mut const_exprs = Arena::new();
    let mut constants = Arena::new();

    let i32_handle = types.insert(Type {
        name: None,
        inner: TypeInner::Scalar { kind: crate::ScalarKind::Sint, width: 4 },
    }, nowhere);

    // Construct a self-referential constant.
    let next_const_index = NonZeroU32::new(constants.len() as u32 + 1).unwrap();
    let self_referential_expr = const_exprs.append(Expression::Constant(Handle::new(next_const_index)), nowhere);
    let _self_referential_const = constants.append(Constant {
        name: None,
        r#override: crate::Override::None,
        ty: i32_handle,
        init: self_referential_expr,
    }, nowhere);

    for handle_and_expr in const_exprs.iter() {
        assert!(super::Validator::validate_const_expression_handles(
            handle_and_expr,
            &constants,
            &types,
        ).is_err());
    }
}

[teoxoy]

I rebased and fixed the compilation warnings when no feature flags are used.

[teoxoy]

Regarding the self-referential test, I'd argue that the new method on Handle should be unsafe. The invariants of the Arenas and Handles shouldn't allow you to create cycles, the only way to get a Handle is to .append().

[jimblandy]

Regarding the self-referential test, I'd argue that the new method on Handle should be unsafe. The invariants of the Arenas and Handles shouldn't allow you to create cycles, the only way to get a Handle is to .append().

We talked about this in chat, and I pushed a new test case that doesn't call Handle::new, along wih a fix.

[jimblandy]

force-pushed cargo fmt fix.

[jimblandy]

Okay, I'm finally done looking this over. With the changes here, this can land.

[teoxoy]

@jimblandy thanks for the review!

I think I addressed all the feedback in the fixup! commits, let me know if something is missing.

[jimblandy]

Follow-up. Still approved.

[jimblandy]

Autosquashed the fixup commits.