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] SHADER_INT64_ATOMIC_MIN_MAX issue #5888

Open
JMS55 opened this issue Jun 27, 2024 · 5 comments
Open

[Naga] SHADER_INT64_ATOMIC_MIN_MAX issue #5888

JMS55 opened this issue Jun 27, 2024 · 5 comments

Comments

@JMS55
Copy link
Contributor

JMS55 commented Jun 27, 2024

@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 JMS55 changed the title [Naga] SHADER_INT64_ATOMIC_MIN_MAX potential issue [Naga] SHADER_INT64_ATOMIC_MIN_MAX issue Jun 27, 2024
@JMS55
Copy link
Contributor Author

JMS55 commented Jun 27, 2024

CC @jimblandy @atlv24

@JMS55
Copy link
Contributor Author

JMS55 commented Jun 28, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants