gfx-rs / wgpu

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

[naga wgsl-out] `let`declarations are wrongly detected as const #6207

Open sagudev opened 2 weeks ago

sagudev commented 2 weeks ago

Description In wgsl-in there is https://github.com/gfx-rs/wgpu/blob/07becfe5b5cc550b04ecbfa303cb6ccfc91357ad/naga/src/front/wgsl/lower/mod.rs#L1370-L1376

but in wgsl-out when we create expression tracker from arena this information is lost: https://github.com/gfx-rs/wgpu/blob/07becfe5b5cc550b04ecbfa303cb6ccfc91357ad/naga/src/back/wgsl/writer.rs#L169 https://github.com/gfx-rs/wgpu/blob/07becfe5b5cc550b04ecbfa303cb6ccfc91357ad/naga/src/back/wgsl/writer.rs#L197

same problem is also in validator https://github.com/gfx-rs/wgpu/blob/07becfe5b5cc550b04ecbfa303cb6ccfc91357ad/naga/src/valid/function.rs#L1368 which causes https://github.com/gfx-rs/wgpu/issues/4390 (such checks should only be done in const but let expr should not be treated as const).

sagudev commented 2 weeks ago

which causes https://github.com/gfx-rs/wgpu/issues/4390

I tested

diff --git a/naga/src/valid/function.rs b/naga/src/valid/function.rs
index 23e6204cc..c60f3a7e9 100644
--- a/naga/src/valid/function.rs
+++ b/naga/src/valid/function.rs
@@ -1365,7 +1365,11 @@ impl super::Validator {
     ) -> Result<FunctionInfo, WithSpan<FunctionError>> {
         let mut info = mod_info.process_function(fun, module, self.flags, self.capabilities)?;

-        let local_expr_kind = crate::proc::ExpressionKindTracker::from_arena(&fun.expressions);
+        let mut local_expr_kind = crate::proc::ExpressionKindTracker::from_arena(&fun.expressions);
+
+        for (handle, _) in fun.expressions.iter() {
+            local_expr_kind.force_non_const(handle);
+        }

         for (var_handle, var) in fun.local_variables.iter() {
             self.validate_local_var(var, module.to_ctx(), &info, &local_expr_kind)

and it doesn't fix, but we should still need correct constness information for fixing the issue.

teoxoy commented 1 week ago

I thought this should be fine for wgsl-out and the validator. The reason it's not ok in the frontend is because treating them as const changes semantics in a non-spec compliant way. What would be an example where this is an issue?

sagudev commented 1 week ago

I do not have concrete example but https://github.com/gfx-rs/wgpu/blob/960555a426b0aae48ed36889b8c7feb48de19a6d/naga/src/back/wgsl/writer.rs#L1113-L1125 could upgrade let expressions to const, which might change behavior (for example when overflows are reported, if we did that correctly).

teoxoy commented 1 week ago

I see, that would be a problem indeed. I think the wgsl backend should just not emit function-local consts at all (that would mean reverting https://github.com/gfx-rs/wgpu/pull/6156/commits/6125ab479056ec2ee859b436a20ed70caac77778 & https://github.com/gfx-rs/wgpu/pull/6156/commits/8e967dbc06cacf9c54969579114f7094cbb71ede). As for the validator, I think it should be fine as we aren't evaluating those expressions and we already check constness in the frontend.