gfx-rs / wgpu

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

[Naga] SHADER_INT64_ATOMIC_MIN_MAX issue #5888

Closed JMS55 closed 2 days ago

JMS55 commented 2 weeks ago
@group(0) @binding(8) var<storage, read_write> meshlet_visibility_buffer: array<atomic<u64>>;

@fragment
fn fragment(@builtin(position) position: vec4<f32>) {
    atomicMax(&meshlet_visibility_buffer[0u], u64(position.z));
}
error: 
  ┌─ foo.wgsl:5:16
  │
5 │     atomicMax(&meshlet_visibility_buffer[0u], u64(position.z));
  │                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ naga::Expression [3]

Entry point fragment at Fragment is invalid: 
        Expression [3] is invalid
        Used by a statement before it was introduced into the scope by any of the dominating blocks

Tested with naga-cli rev 82210e1c

JMS55 commented 2 weeks ago

CC @jimblandy @atlv24

JMS55 commented 2 weeks ago

Backtrace:

 4: naga::valid::function::BlockContext::resolve_type_impl
             at D:\wgpu\naga\src\valid\function.rs:261
   5: naga::valid::function::BlockContext::resolve_type
             at D:\wgpu\naga\src\valid\function.rs:276
   6: naga::valid::Validator::validate_atomic
             at D:\wgpu\naga\src\valid\function.rs:372
   7: naga::valid::Validator::validate_block_impl
             at D:\wgpu\naga\src\valid\function.rs:1144
   8: naga::valid::Validator::validate_block
             at D:\wgpu\naga\src\valid\function.rs:1325
   9: naga::valid::Validator::validate_function
             at D:\wgpu\naga\src\valid\function.rs:1471
  10: naga::valid::Validator::validate_entry_point
             at D:\wgpu\naga\src\valid\interface.rs:606
  11: naga::valid::Validator::validate_impl
             at D:\wgpu\naga\src\valid\mod.rs:713
  12: naga::valid::Validator::validate
             at D:\wgpu\naga\src\valid\mod.rs:563
jimblandy commented 2 weeks ago

Okay, so this is a front-end bug. We're getting the following IR:

                expressions: {
                    [0]: FunctionArgument(
                        0,
                    ),
                    [1]: GlobalVariable(
                        [0],
                    ),
                    [2]: AccessIndex {
                        base: [1],
                        index: 0,
                    },
                    [3]: AccessIndex {
                        base: [0],
                        index: 2,
                    },
                    [4]: As {
                        expr: [3],
                        kind: Uint,
                        convert: Some(
                            8,
                        ),
                    },
                },
                named_expressions: {
                    [0]: "position",
                },
                body: Block {
                    body: [
                        Atomic {
                            pointer: [2],
                            fun: Max,
                            value: [4],
                            result: None,
                        },
                        Emit(
                            [2..5],
                        ),
                    ],

That Atomic statement is referring to expression [2], the first AccessIndex instruction - and that indeed is not covered by any Emit statement.

jimblandy commented 2 weeks ago

It seems like, by deciding not to interrupt the emitter to produce an AtomicResult expression, we don't produce an Emit statement before the Atomic statement at all.

But then I can't explain why the existing test case naga/tests/in/atomicOps-int64-min-max.wgsl doesn't hit this problem as well.

jimblandy commented 2 weeks ago

This seems to fix the problem, but that doesn't account for the above.

modified   naga/src/front/wgsl/lower/mod.rs
@@ -2482,6 +2482,9 @@ impl<'source, 'temp> Lowerer<'source, 'temp> {
                     crate::TypeInner::Scalar(crate::Scalar { width: 8, .. })
                 );
         let result = if is_64_bit_min_max && is_statement {
+            let rctx = ctx.runtime_expression_ctx(span)?;
+            rctx.block.extend(rctx.emitter.finish(&rctx.function.expressions));
+            rctx.emitter.start(&rctx.function.expressions);
             None
         } else {
             let ty = ctx.register_type(value)?;