gfx-rs / wgpu

A cross-platform, safe, pure-Rust graphics API.
https://wgpu.rs
Apache License 2.0
12.4k stars 908 forks source link

WGSL parser not always start validating from the correct expression #4926

Open Norlock opened 9 months ago

Norlock commented 9 months ago

Description It seems that the WGSL parser doesn't always start validating from the correct expression. First example code:

@compute
@workgroup_size(16, 16)
fn generate_terrain(@builtin(global_invocation_id) position: vec3<u32>) {
    if any(vec2<u32>(terrain.group_size) <= position.xy) {
        return;
    }

And the error message:

Shader validation error: 
    ┌─ Terrain SDR:478:8
    │
478 │ if any(vec2<u32>(terrain.group_size) <= position.xy) {
    │        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ naga::Expression [7]

    Entry point generate_terrain at Compute is invalid
    Expression [7] is invalid
    Operation LessEqual can't work with [5] and [6]

Which is wrong, because the error is that terrain.group_size is an f32 which it can't cast to a vec2<u32>.

This is another example:

477 │ fn generate_terrain(@builtin(global_invocation_id) position: vec3<u32>) {
    │                                                    ^^^^^^^^ naga::Expression [1]

    Entry point generate_terrain at Compute is invalid
    Image store parameters are invalid
    Image coordinate type of [1] does not match dimension D2

Which should fail on: textureStore(cube_write, position, vec4(1.0));. Because the expression that fails should be argument 2 of the function call.

Repro steps

git clone https://github.com/Norlock/sparticles.git
git checkout bug-wgsl
cargo run --release

Shader file: https://github.com/Norlock/sparticles/blob/849ff9ef5f4681a70d9538cb81ed32d5a00ea0a0/crates/sparticles_app/src/shaders/terrain/terrain.wgsl

Expected vs observed behavior I expect the parser to start checking the smallest possible expression in the tree and move up. So in case of:

478 │ if any(vec2<u32>(terrain.group_size) <= position.xy) {

It will start by checking the cast expression and then the smaller than expression.

Platform Linux, egui-wgpu 24.0 -> 0.18

Norlock commented 9 months ago

I would also like to fix it myself if someone can explain me a bit where to start and a give me a short introduction of the code, so I can pick it more quickly.

jimblandy commented 9 months ago

@Norlock The WGSL front end is in src/front/wgsl. The parser produces a naga::front::wgsl::parse::ast::TranslationUnit, which doesn't have identifiers resolved to their definitions yet. naga::front::wgsl::lower::Lowerer::lower is the function that takes that TranslationUnit and builds a naga::Module from it. Most of the interesting work happens there, so that's where I would expect to find the problem.

Module has extensive (but not exhaustive) documentation in naga/src/lib.rs.

I would recommend using the Naga CLI (naga-cli/src/bin/naga.rs) to experiment with your changes. You can ask it to produce a .txt file as output; that will be a dump of the Naga IR Module itself.

Norlock commented 9 months ago

Ok thanks for the advice I will try to look at it tomorrow