wgpu icon indicating copy to clipboard operation
wgpu copied to clipboard

[Naga] SHADER_INT64_ATOMIC_MIN_MAX issue

Open JMS55 opened this issue 1 year ago • 5 comments

@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 avatar Jun 27 '24 05:06 JMS55

CC @jimblandy @atlv24

JMS55 avatar Jun 27 '24 05:06 JMS55

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

JMS55 avatar Jun 28 '24 17:06 JMS55

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 avatar Jun 28 '24 18:06 jimblandy

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 avatar Jun 28 '24 19:06 jimblandy

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)?;

jimblandy avatar Jun 28 '24 19:06 jimblandy