[wgsl-in] Implement template list discovery and handle all type resolution in the lowerer - #8386
Conversation
9c99aa1 to
d4b4df4
Compare
|
I can take review here - I've been looking into this for a long time. |
|
So far I've reviewed the following commits and they all look fine: |
5f4abe6 to
13a9c1b
Compare
|
I removed the last commit that was adding a CTS test since it wasn't passing due to unrelated reasons and pushed 2 more commits to fix the issues CI surfaced. |
They could but that would be lots of separate PRs to file and orchestrate since I think some of them depend on others. I could open a PR just with the refactor commits but then this PR would need to depend on that new one. Let me know. |
jimblandy
left a comment
There was a problem hiding this comment.
All the refactoring changes up to 4ebbc67df * implement template list discovery look good. I think there's a bug in the TLD code itself.
|
I pushed a version of this that is rebased on top of #7339 to https://github.com/andyleiserson/wgpu/commits/tld-rebased. The last commit resolves a semantic merge conflict with #7339. There were also merge conflicts with trunk in 314ba9b and cf9e111 that I resolved, and there is a merge conflict with #8545 that I was less sure how to handle (that change adds enable extension checks that need to be relocated somewhere else following the template list discovery changes). |
|
@andyleiserson thanks for tracking the changes! I pushed a fully rebased version. |
d10766f to
5b76532
Compare
|
I pushed two very minor tweaks from today's reviewing. |
|
The first one looks good but the 2nd one assumes that those statements always end with a semicolon. Off the top of my head they are also used in the for loop header. |
|
I pushed two very minor tweaks from today's reviewing.
Okay, right - I fixed these. Pushed a few more, and some tweaks, please take a look. |
|
The template list discovery commit looks good to me. |
This improves errors making them more general where appropriate.
In `naga::front::wgsl::parse::Parser::component_or_swizzle_specifier`, since `span_start` sounds like the start of a span, rename that argument to `expr_start`, since it is the span of the start of the expression that ends with the component or swizzle specifier.
|
@teoxoy Before we merge this, could you take a quick look at the way cooperative matrix types and functions are handled? I think just looking at the tip should be fine, no need to sort through all the commits. |
|
I just gave all the cooperative stuff a quick look, and I didn't notice anything that looked like it was doing things the old way.
|
Avoid adding entries to `ExpressionContext::unresolved` for identifiers that `ExpressionContext::ident_expr` has already processed. At the moment, there is no way these could cause a bug: they only occur after we've recognized a call/construction expression, and those can never refer to function-local definitions, so the string in question is certain to have already been added to `unresolved` by `ExpressionContext::ident_expr`. But they're redundant, and if Naga ever supported nested functions or local type aliases, these could end up adding extraneous dependencies.
The name `scalar` suggests that it returns a scalar value, and `scalar_ty` is analogous to `ty`.
- Clarify that various `foo_statement` functions on `Parser` do not consume the statement's final `;`. - Various other doc fixes throughout.
Use `if let`, `let else`, and `Option` methods to reduce nesting and clarify success paths.
Rather than passing `call_impl` an `ignore_builtin_identifiers` flag and plumbing that through from its points of use, extend `Namer::namespace` to suspend the `builtin_identifiers` table, as it does for `unique` already. The resulting behavior is the same.
Adjust some methods of `TemplateListIter` to ensure that the typical complete control flow through the function follows the outermost nesting level.
Rename `naga::front::wgsl::error::Error::MissingTemplateArg::arg` to `description`, to make it a little more specific.
|
Since you marked this as "rebase", I consolidated my "doc fix" commits a bit, to keep the history cleaner. |
|
I have got to stop fiddling with this PR |
|
The cooperative matrix stuff looks good to me; as you mentioned it looks to me like we handle them the same way as the other predeclared types & builtin functions. |
|
Let's land this 🎉 |
Connections
Resolves #4501.
Resolves #4406.
Resolves #4405.
Description
Almost all commits prior to
implement template list discoveryare refactors to get the frontend to look more similar to the spec and to make the needed changes easier to implement (most notably making theTokenizerno longer implClone).The template list discovery implementation differs from the spec only in structure (not in behavior) since we can do it lazily after tokenization.
The last 3 substantial commits move all type resolution to the lowerer since all identifiers that are part of types need to be resolved the same as all other identifiers.
Testing
Changed existing tests and added new ones.
Squash or Rebase?
Rebase.