search

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

`LocalVariable::init` is barely used #4488

Open jimblandy opened 1 year ago

jimblandy commented 1 year ago

The init field of naga::LocalVariable is barely used by any front ends. It would be nice to delete it.

Initializers for local variables in GLSL and WGSL can be arbitrary expressions. Since Naga IR effectively hoists all function-local variables up to the top of the function, front ends turn those initializers into Store statements executed at the point at which the variable gets declared in the original source. This means that LocalVariable's own init field isn't good for much.

In fact, the only front end that puts anything other than None in LocalVariable::init is GLSL, which uses it for some indexing hack that I don't understand. Surely that could be rewritten, isolating that complexity to the front end that needs it.

jimblandy commented 1 year ago

The GLSL front end hack is working around Naga IR's lack of support for dynamically indexing arrays and matrices that are not behind a pointer (test). This IR restriction reflected a similar one in WGSL, but that was lifted in https://github.com/gpuweb/gpuweb/pull/2427. gfx-rs/wgpu#4337 was filed to bring Naga into compliance. If that is implemented, then the GLSL front end hack can be removed, the final use of LocalVariable::init will go away, and it can be removed.

teoxoy commented 1 year ago

The initializer of local variables is mainly for the spir-v backend (and frontend) since it can only be a constant.

We were also previously setting the initializer in the WGSL frontend but this changed:

The only difference in output is that I don't initialize LocalVariables with constants since I'm not too sure if that added complexity is actually worth it. I can add it back if it's something wanted, though.

from https://github.com/gfx-rs/naga/pull/2075#issuecomment-1304453265

I was fine with removing this temporarily since this is something that we can easily add back with the introduction of a constant evaluator.

By removing the initializer there will be instances where we will initialize local variables twice and I'm not sure if all drivers will optimize the first initialization.

@kvark also had this concern here:

Another option would be to remove the serializer entirely from the local variables... which is fine, semantically speaking, since everything is expected to be zero-initialized, at least in wgpu. It may be less fine from the performance perspective, since assigning a value would map to OpStore, while the initializer is free in SPIR-V.

from https://github.com/gfx-rs/naga/issues/97#issuecomment-720900533

I think we should keep the initializer unless we have some numbers showing that it's irrelevant.