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

Implement override-expression evaluation in functions #5387

Merged
merged 12 commits into from
Mar 28, 2024

Conversation

teoxoy
Copy link
Member

@teoxoy teoxoy commented Mar 14, 2024

Tracking meta issue: #4484.

@teoxoy teoxoy requested a review from a team as a code owner March 14, 2024 17:14
@jimblandy
Copy link
Member

I think the try_eval_and_append refactor is a good improvement.

@jimblandy
Copy link
Member

@teoxoy I've pushed some new commits to the branch. Could you look them over?

I'm not sure about "Hoist ConstantEvaluator". It does make the loop body simpler, but maybe it's nice to make it explicit that these ConstantEvaluators don't retain any state from one iteration to the next?

Comment on lines +275 to +278
// Dummy `emitter` and `block` for the constant evaluator.
// We can ignore the concept of emitting expressions here since
// expressions have already been covered by a `Statement::Emit`
// in the frontend.
Copy link
Member

@jimblandy jimblandy Mar 23, 2024

Choose a reason for hiding this comment

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

Super-helpful. At the exact point where I'm thinking, "What in the world?", the comment points out to me what I was missing, and I can just move on.

Comment on lines +293 to +301
for (old_h, mut expr, span) in expressions.drain() {
if let Expression::Override(h) = expr {
expr = Expression::Constant(override_map[h.index()]);
}
adjust_expr(&adjusted_local_expressions, &mut expr);
let h = evaluator.try_eval_and_append(expr, span)?;
debug_assert_eq!(old_h.index(), adjusted_local_expressions.len());
adjusted_local_expressions.push(h);
}
Copy link
Member

Choose a reason for hiding this comment

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

The simplicity of this loop is encouraging, as it suggests that changing when constant evaluation occurs would not be such a big change: you'd simply have a loop like this after validation.

@jimblandy
Copy link
Member

@teoxoy Does this close #4366 and #1762?

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.
@jimblandy
Copy link
Member

Accidentally whacked the upstream branch.

@jimblandy jimblandy reopened this Mar 25, 2024
@jimblandy
Copy link
Member

Found some expression handles that weren't getting adjusted.

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.

Okay! Please review my commits before merging, but with those fixes, this PR looks good to me.

@teoxoy
Copy link
Member Author

teoxoy commented Mar 28, 2024

Does this close #4366 and #1762?

Yeah but let's do that once we merge the branch into trunk.

@teoxoy
Copy link
Member Author

teoxoy commented Mar 28, 2024

Found some expression handles that weren't getting adjusted.

Good catch! We can hopefully avoid those cases once we have some common infrastructure to deal with IR mutation.

@teoxoy
Copy link
Member Author

teoxoy commented Mar 28, 2024

Okay! Please review my commits before merging, but with those fixes, this PR looks good to me.

They look good, thanks!

@teoxoy teoxoy merged commit c7bcd9c into gfx-rs:pipeline-constants Mar 28, 2024
27 checks passed
@teoxoy teoxoy mentioned this pull request Apr 5, 2024
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.

2 participants