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

[wgsl-in] Allow sign() to take int argument #2463

Merged
merged 4 commits into from
Sep 6, 2023

Conversation

fornwall
Copy link
Contributor

@fornwall fornwall commented Sep 5, 2023

Fixes #2458.

The main work here is to make it work on metal, which does not have an int overload of sign(). So we introduce a naga_isign polyfill function there.

Currently the metal backend always emits the naga_isign polyfill. What is a good way to only do that if necessary? We need to declare the function before it's used, so we can't do it after writing out the functions. Is there some mechanism to visit all expressions without duplicating a lot of logic?

ErichDonGubler

This comment was marked as duplicate.

Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

This mostly LGTM (noting that I don't have merge authority), minus nits and one question: would it be possible to omit this new polyfill unless needed? I'm not familiar with the matter, but I know we've thought about lazily emitting supporting polyfills WRT constructing array members of HLSL structures before. EDIT: 😅 I see that you're also asking this question in the OP:

Currently the metal backend always emits the naga_isign polyfill. What is a good way to only do that if necessary? We need to declare the function before it's used, so we can't do it after writing out the functions. Is there some mechanism to visit all expressions without duplicating a lot of logic?

☝🏻I think @jimblandy or @teoxoy would know the answer to this.

src/back/msl/writer.rs Outdated Show resolved Hide resolved
src/back/msl/writer.rs Outdated Show resolved Hide resolved
src/valid/expression.rs Outdated Show resolved Hide resolved
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.

Rather than generating a definition for a polyfill, consider simply emitting the code inline. See the way the Metal backend handles MathFunction::Dot for an example:

  • It calls back::msl::Writer::put_dot_product to emit the expression at the point of use.
  • In back::msl::Writer::update_expressions_to_bake, we ensure that the operand is placed in a temporary variable first, so that we don't evaluate the argument expression more than once.

tests/in/math-functions.wgsl Outdated Show resolved Hide resolved
src/back/msl/writer.rs Outdated Show resolved Hide resolved
@fornwall fornwall force-pushed the wgsl-in-fix-sign branch 3 times, most recently from e209bd9 to d64d40c Compare September 6, 2023 06:47
@fornwall
Copy link
Contributor Author

fornwall commented Sep 6, 2023

Rather than generating a definition for a polyfill, consider simply emitting the code inline.

Thanks for the pointers @jimblandy! Changed to emitting inline in 32e96a5.

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.

Thank you very much!

@jimblandy jimblandy merged commit a0eb1f5 into gfx-rs:master Sep 6, 2023
9 checks passed
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.

[wgsl-in] sign() incorrectly does not accept i32 arguments
3 participants