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/wgsl-out]: polyfill inverse function #6385

Merged
merged 3 commits into from
Oct 11, 2024

Conversation

chyyran
Copy link
Contributor

@chyyran chyyran commented Oct 9, 2024

Connections

Description
inverse is not currently supported by WGSL and might never be. However this leaves shaders that do need these functions fundamentally incompatible with WGSL unless they get polyfilled. This PR allows wgsl-out to emit polyfills for GLSL inverse functions.

The polyfills provided are ported from an old Mesa commit, and have been pre-validated with naga-cli.

outer is not included as part of this patch as it's much more rarer than inverse, and another approach would probably be better. For example SPIRV-Cross just expands the calculation inline.

Pre-emptive support for f16 is included as well. Note that the polyfill files are intentionally not templated to reduce large allocations during shader compilation. This allows InversePolyfill to be 2 ’static pointers and no larger.

Testing
Added an inverse-polyfill.frag test to the in/glsl folder.

This test is then run to validate the changes.

$ cargo test convert_glsl_folder --features="glsl-in,wgsl-out"

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@chyyran chyyran requested a review from a team as a code owner October 9, 2024 03:47
@chyyran chyyran force-pushed the wgsl-out-polyfill-inverse branch 2 times, most recently from 0cb32f3 to ba0b5b3 Compare October 9, 2024 03:51
naga/src/back/wgsl/writer.rs Outdated Show resolved Hide resolved
naga/src/back/wgsl/writer.rs Outdated Show resolved Hide resolved
@chyyran chyyran force-pushed the wgsl-out-polyfill-inverse branch from ba0b5b3 to 0c93797 Compare October 10, 2024 21:00
@chyyran
Copy link
Contributor Author

chyyran commented Oct 10, 2024

Fixed clippy errors and applied the suggestion from @teoxoy. Will await consensus from @jimblandy before moving forward with whether to keep the flag. I don't mind either way, but the idea to have the flag stemmed from concerns in #4330 where OP seemed to have reservations about the injected polyfill being "quite long".

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, the polyfill names all need to start with __, a double underscore. A single underscore isn't a reserved prefix (see back::wgsl::Writer::reset's call to Namer::reset), so these injected polyfills could conflict with a definition supplied by the user for _naga_inverse_blah.

@chyyran chyyran force-pushed the wgsl-out-polyfill-inverse branch 3 times, most recently from 7a37872 to c4b141c Compare October 10, 2024 22:49
@chyyran
Copy link
Contributor Author

chyyran commented Oct 10, 2024

@jimblandy Made the emitted polyfills start with a double underscore and further namespaced them as __naga_polyfill. I've had to add an exception to wgsl-in to allow __naga_polyfill in the lexer otherwise wgsl-out would output WGSL that wgsl-in would reject.

@chyyran chyyran requested review from teoxoy and jimblandy October 10, 2024 22:50
@jimblandy
Copy link
Member

@chyyran No, you're right, double underscore isn't something that the code that we generate can define. We should be producing WGSL that other WGSL implementations would accept. Sorry for the bad advice.

I think instead you should change the polyfill names back, so they start with _naga again, and just add "_naga" to the list of reserved prefixes that the WGSL back end passes to Namer::reset. The namer will take care of consistently adjusting any identifiers coming from the naga::Module, so that should just work. Then we can use _naga in the WGSL backend for any future injected stuff.

@chyyran chyyran force-pushed the wgsl-out-polyfill-inverse branch from c4b141c to 8309067 Compare October 10, 2024 23:14
@chyyran
Copy link
Contributor Author

chyyran commented Oct 10, 2024

@jimblandy dropped the changes to add the double underscore and added _naga to the list of reserved prefixes in Namer in 8309067

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@teoxoy
Copy link
Member

teoxoy commented Oct 11, 2024

@chyyran it looks like the snapshots need updating.

@chyyran
Copy link
Contributor Author

chyyran commented Oct 11, 2024

Looks like its sensitive to insertion order. I’ll switch it to use an indexset.

@chyyran chyyran force-pushed the wgsl-out-polyfill-inverse branch from 8309067 to 083d7e9 Compare October 11, 2024 12:39
@chyyran
Copy link
Contributor Author

chyyran commented Oct 11, 2024

@teoxoy switched required_polyfills to an indexset in a0a81ac, could you rerun workflows?

@teoxoy teoxoy dismissed jimblandy’s stale review October 11, 2024 13:55

concern was addressed

@teoxoy teoxoy merged commit 73764fd into gfx-rs:trunk Oct 11, 2024
27 checks passed
@chyyran chyyran deleted the wgsl-out-polyfill-inverse branch October 15, 2024 01:06
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

Successfully merging this pull request may close these issues.

3 participants