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.

Sign up for GitHub

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

Jump to bottom

Pipeline constants #5500

Merged
merged 30 commits into from
Apr 5, 2024
Merged

Pipeline constants #5500

merged 30 commits into from
Apr 5, 2024

Conversation

teoxoy
Copy link
Member

@teoxoy teoxoy commented Apr 5, 2024

It's time 🎉

Resolves #4366 & resolves #1762.

All commits have been previously approved in the following PRs:

There are still 2 features that remain to be implemented:

Commits should not be squashed (they are self-contained and passed CI individually).

teoxoy and others added 29 commits April 4, 2024 11:36
@teoxoy
add pipeline constants plumbing
5ec78ab
@jimblandy @teoxoy
[naga] Delete Constant::override and Override.
747fb30
@teoxoy @jimblandy
[wgsl-in] add support for override declarations (#4793)
e5b7df3 
Co-authored-by: Jim Blandy <jimb@red-bean.com>
@teoxoy
remove naga's clone feature
da0a6c9
@teoxoy
[spv/msl/hlsl-out] support pipeline constant value replacements
1a3c47b
@teoxoy
validate that override ids are unique
d340f9f
@teoxoy
[const-eval] refactor logic around try_eval_and_append
120d0a0
@teoxoy
[const-eval] fix evaluation of bool constuctors
e9775c8
@teoxoy
implement override-expression evaluation for initializers of override…
c48f01b 
… declarations
@teoxoy
rename ExpressionConstnessTracker to ExpressionKindTracker
98f4ca0
@teoxoy
rename const_expressions to global_expressions
29abd93
@teoxoy
[valid] error on non fully evaluated const-expressions
bb999fc
@teoxoy
[valid] make sure overrides are not present after evaluation
7d79d71
@jimblandy @teoxoy
[naga] Add some documentation to process_overrides and subroutines.
f3ff6b3
@teoxoy
refactor try_eval_and_append body
0c4b2d7
@teoxoy
evaluate override-expressions in functions
e2ff98b
@teoxoy
allow private variables to have an override-expression initializer
e2fcc26
@jimblandy @teoxoy
[naga] Doc tweaks for back::pipeline_constants.
dcef6c6
@jimblandy @teoxoy
[naga] Simplify uses of replace in back::pipeline_constants.
df8c8cb
@jimblandy @teoxoy
[naga] Hoist ConstantEvaluator construction in process_function.
106f56a 
There's no need to build a fresh `ConstantEvaluator` for every
expression; just build it once and reuse it.
@jimblandy @teoxoy
[naga] Let filter_emits_with_block operate on a &mut Block.
ce77287 
This removes some clones and collects, simplifies call sites, and
isn't any more complicated to implement.
@jimblandy @teoxoy
[naga] Tweak comments in ConstantEvaluator::try_eval_and_append.
2c29ecb 
I found I needed a little bit more detail here.
@jimblandy @teoxoy
[naga] Add missing newline to test input file.
e62621b
@jimblandy @teoxoy
[naga] Handle comparison operands in pipeline constant evaluation.
ddd1222 
Properly adjust `AtomicFunction::Exchange::compare` after pipeline
constant evaluation.
@jimblandy @teoxoy
[naga] Spell out members in adjust_expr.
f015fb1
@jimblandy @teoxoy
[naga] Adjust RayQuery statements in override processing.
de50eb0
@jimblandy @teoxoy
[naga-cli] Add --override option.
b9403c1
@teoxoy @jimblandy
move the burden of evaluating override-expressions to users of naga's…
9c3fc6c 
… API
@jimblandy
[naga wgsl-in] Allow override expressions as local var initializers.
c104f08 
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 as a code owner April 5, 2024 15:07
@teoxoy teoxoy requested a review from a team as a code owner April 5, 2024 15:07
jimblandy
jimblandy approved these changes Apr 5, 2024
View reviewed changes
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.

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

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

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

@DJMcNab DJMcNab mentioned this pull request Apr 10, 2024
Merged
10 tasks
@hannesojala
Copy link

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
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

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

@DouglasDwyer DouglasDwyer mentioned this pull request May 11, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimblandy jimblandy jimblandy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pipeline-overridable constants Pipeline overrides
5 participants
@teoxoy @AdrianEddy @hannesojala @ErichDonGubler @jimblandy