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

LocalVariable::init is barely used #4488

Open
jimblandy opened this issue Apr 4, 2023 · 2 comments
Open

LocalVariable::init is barely used #4488

jimblandy opened this issue Apr 4, 2023 · 2 comments
Labels
area: naga front-end area: naga middle-end Intermediate representation kind: refactor Making existing function faster or nicer lang: GLSL OpenGL Shading Language naga Shader Translator

Comments

@jimblandy
Copy link
Member

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 jimblandy added kind: refactor Making existing function faster or nicer area: naga front-end area: naga middle-end Intermediate representation lang: GLSL OpenGL Shading Language labels Apr 4, 2023
@jimblandy
Copy link
Member Author

jimblandy commented Apr 4, 2023

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 gpuweb/gpuweb#2427. #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
Copy link
Member

teoxoy commented Apr 4, 2023

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 gfx-rs/naga#2075 (comment)

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 gfx-rs/naga#97 (comment)

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

@cwfitzgerald cwfitzgerald added the naga Shader Translator label Oct 25, 2023
@cwfitzgerald cwfitzgerald transferred this issue from gfx-rs/naga Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: naga front-end area: naga middle-end Intermediate representation kind: refactor Making existing function faster or nicer lang: GLSL OpenGL Shading Language naga Shader Translator
Projects
None yet
Development

No branches or pull requests

3 participants