Skip to content

Pipeline constants#5500

Merged
teoxoy merged 30 commits intotrunkfrom
pipeline-constants
Apr 5, 2024
Merged

Pipeline constants#5500
teoxoy merged 30 commits intotrunkfrom
pipeline-constants

Conversation

@teoxoy
Copy link
Copy Markdown
Member

@teoxoy teoxoy commented Apr 5, 2024

teoxoy and others added 29 commits April 4, 2024 11:36
Co-authored-by: Jim Blandy <jimb@red-bean.com>
There's no need to build a fresh `ConstantEvaluator` for every
expression; just build it once and reuse it.
This removes some clones and collects, simplifies call sites, and
isn't any more complicated to implement.
I found I needed a little bit more detail here.
Properly adjust `AtomicFunction::Exchange::compare` after pipeline
constant evaluation.
Allow `LocalVariable::init` to be an override expression.

Note that this is unrelated to WGSL compliance. The WGSL front end
already accepts any sort of expression as an initializer for
`LocalVariable`s, but initialization by an override expression was
handled in the same way as initialization by a runtime expression, via
an explicit `Store` statement.

This commit merely lets us skip the `Store` when the initializer is an
override expression, producing slightly cleaner output in some cases.
@teoxoy teoxoy requested a review from a team April 5, 2024 15:07
@teoxoy teoxoy requested a review from a team as a code owner April 5, 2024 15:07
Copy link
Copy Markdown
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.

All these changes have been previously reviewed as PRs to the pipeline-constants branch.

@teoxoy teoxoy merged commit b985f16 into trunk Apr 5, 2024
@teoxoy teoxoy deleted the pipeline-constants branch April 5, 2024 16:07
@AdrianEddy
Copy link
Copy Markdown
Contributor

Wooo! Thanks for all your efforts, I've been waiting for this for a long time! Great job!

@hannesojala
Copy link
Copy Markdown

This is awesome! Is there a reason that the constants field isn't an Option<_>? Haven't thought too deeply about this, just curious.

@ErichDonGubler
Copy link
Copy Markdown
Member

@hannesojala: Well, let's start thinking, then! 😉 Why would you want an Option? Does it offer something you expected that HashMap does not?

AFAIK, HashMap affords everything one might want, particularly an implementation of Default.

@hannesojala
Copy link
Copy Markdown

That makes perfect sense! I was only thinking from a semantic clarity perspective, but that's not everything, of course.

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.

Pipeline-overridable constants Pipeline overrides

5 participants