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

Initial GLSL constant evaluation implementation #94

Merged
merged 1 commit into from
Jul 10, 2020

Conversation

kristoff3r
Copy link
Contributor

Adds constant evaluation for addition, and uses it for globals and array
specifiers.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Looks nice!

@kristoff3r
Copy link
Contributor Author

kristoff3r commented Jul 2, 2020

Why doesn't my clippy give the same warnings as this one? It's the same version and everything :/

@kristoff3r kristoff3r marked this pull request as ready for review July 2, 2020 22:13
@Timo-DK
Copy link
Collaborator

Timo-DK commented Jul 2, 2020

@kristoff3r that problem I am encountering too, this is because we use features now in Naga, and Clippy doesn't scan by default those features. I use the same command the CI is doing too, which is cargo clippy --all-features. That one should give you the errors/warnings.

@kristoff3r
Copy link
Contributor Author

@kristoff3r that problem I am encountering too, this is because we use features now in Naga, and Clippy doesn't scan by default those features. I use the same command the CI is doing too, which is cargo clippy --all-features. That one should give you the errors/warnings.

I already did, but apparently clippy doesn't re-check if the project is already built, and I had a cargo watch running in another terminal. I ended up doing touch src/lib.rs; cargo clippy --all-features.

@pjoe
Copy link
Collaborator

pjoe commented Jul 3, 2020

I haven't yet figured out how to do the semantic parsing in glsl-new. I'd like to re-use as much as possible from here, but need to understand the code better first 😄

Constant evaluation looks cool though.

One thing I'm still a bit unclear about is how much 'optimization' makes sense in naga - my gut feeling is that some are best left to the consumer of the output as I expect drivers or whatever the consumer is will already run it's own set of optimizations. Especially when considering naga binary (wasm) size 🤏 That being said this kind of constant folding makes total sense.

@pjoe
Copy link
Collaborator

pjoe commented Jul 3, 2020

One more thing to consider: when possible I would prefer 'optimizations' to happen at the naga IR level when possible. That way they can be implemented once instead of for each front end 😃

@kristoff3r
Copy link
Contributor Author

@pjoe I was thinking the same thing when I wrote it. Ideally we want to do as little evaluation/optimization as possible in the frontend, but for array sizes the Naga IR requires actual integers so we have to evaluate those expressions. I added the skeleton to do arbitrary constant expressions, but I hope we can make do with integral constant expressions as that is quite a bit simpler.

(Also, I just noticed we apparently live in the same city. Small world :D)

@kvark
Copy link
Member

kvark commented Jul 3, 2020

@kristoff3r wait a sec. The IR is not perfect either. I believe WGSL is meant to have constant evaluation too.
In this case, we should improve the IR. Edit: filed #97

How does this sound to you? Would you be interested in trying to roll this in?

@kristoff3r
Copy link
Contributor Author

@kvark that was the other option which I considered, but which I forgot to write down here. That seems sensible, and I think I can re-use most of what I have done here, so I'll try to migrate to that solution.

@kvark
Copy link
Member

kvark commented Jul 8, 2020

@kristoff3r let's proceed with your PR. Is it all ready to land?

@kvark
Copy link
Member

kvark commented Jul 8, 2020

Perhaps, you could rebase it to see how CI feels now. There have been changes to the IR that I landed recently.

@kristoff3r
Copy link
Contributor Author

Ok, I'll do that later tonight.

Adds constant evaluation for addition, and uses it for globals and array
specifiers.
Copy link
Member

@kvark kvark 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!

expr: Handle<Expression>,
expressions: &Arena<Expression>,
) -> Result<Handle<Constant>, Error> {
match &expressions[expr] {
Copy link
Member

Choose a reason for hiding this comment

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

note: we strongly prefer to match on values everywhere, not references

@kvark kvark merged commit 6c06208 into gfx-rs:master Jul 10, 2020
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.

4 participants