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

Compile with context #5791

Closed
wants to merge 11 commits into from
Closed

Conversation

stefnotch
Copy link
Contributor

@stefnotch stefnotch commented Jun 10, 2024

Connections
Fix #5713

Description
This makes it possible to take a compiled module, and use it as a "base" for compiling another module. The semantics is

  1. Compile the base module. Assume it exports every global (function, struct, alias, etc).
  2. Compile the next module, and give it every global from the base module.

To only make individual items importable, one can first pre-process the base module.

  1. Generate UUID
  2. Iterate over every global in the base module, and rename it to "UUID_name_of_the_global".

This particular implementation avoids exposing internals that weren't exposed before.

Testing
I added a comprehensive list of tests to naga/src/front/wgsl/tests.rs . There are 3 commented out tests that I would like to add, but currently cannot, due to other naga limitations. How should I best deal with those?

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@stefnotch stefnotch requested a review from a team June 10, 2024 07:31
.insert(entry_point.name.as_str(), LoweredGlobalDecl::EntryPoint);
}
*ctx.global_expression_kind_tracker =
crate::proc::ExpressionKindTracker::from_arena(&ctx.module.global_expressions);
Copy link
Contributor Author

@stefnotch stefnotch Jun 10, 2024

Choose a reason for hiding this comment

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

Restoring the context. Most parts of the lowerer's context do not need to be restored, from what I could gather.

source: &'a str,
translation_unit: TranslationUnit<'a>,
index: index::Index<'a>,
}
Copy link
Contributor Author

@stefnotch stefnotch Jun 10, 2024

Choose a reason for hiding this comment

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

This intermediate struct lets us repeatedly compile a module with different base modules. That's something that naga_oil may want, hence this API.

@jimblandy
Copy link
Member

jimblandy commented Jun 21, 2024

@stefnotch What is this feature used for, exactly?

Where is this used in production?

Comment on lines +98 to +100
- Compile shaders with a "base module". This allows for cleanly implementing imports, or shadertoy-like environments. By @stefnotch in [#5791](https://github.com/gfx-rs/wgpu/pull/5791).
```rust
use naga::front::wgsl::Frontend;
Copy link
Member

Choose a reason for hiding this comment

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

We can't put documentation in CHANGELOG.md. If the feature is to be part of Naga, it has to be documented properly, the way we try to do for the rest of Naga.

@jimblandy
Copy link
Member

jimblandy commented Jun 21, 2024

I think the main problem this PR is trying to address is that the WGSL front end really only operates on complete modules. If all the definitions aren't present, then the front end can't even lower the ast form to a Module. In fact, Naga's Module type itself is designed to ensure that all references to types, functions, variables, etc. are fully resolved and point directly to their definitions, which makes it useless for representing chunks of code that haven't yet been combined into a complete program.

If someone wants to experiment with WGSL module designs, they'd like to be able to borrow Naga's WGSL parser, rather than rewriting one from scratch, but since naga::front::wgsl wants to run Index::generate and figure out what everything refers to, that's kind of hard. It's a real impediment to experimentation.

My concern about this PR is that it doesn't seem like it really accomplishes much more than:

  • validating the base module to make sure it's valid WGSL
  • concatenating the base module source code with more code and validating again

In other words, it saves you having to re-parse the module, but it doesn't really enable new ways of using the code.

It's got a bunch of other issues, too, like, Naga spans don't have source ids, so if you have multiple source files Naga has no way to indicate which one a given span belongs to.

So although I think that this is an absolutely fantastic avenue for experimentation, and definitely a direction that's worth exploring, as it stands this doesn't look like something that is mature enough to merge into trunk.

If you would like to have a branch in gfx-rs/wgpu so that you can collaborate with others as you experiment with this design and others, that's definitely something we would be happy to do.

I definitely think we need to work with Bevy and others to understand what they're doing with their templating system, and set up some kind of process where we can iterate with developers to extend WGSL to better support complex shader development. It's something the WebGPU standards committee members also have very much on their minds, it came up in our office hours hangouts. But people have widely varying ideas about what support for complex shaders should look like; one person felt that C++-style if constexpr was the way forward, whereas I thought something more like traits and generics would serve better.

The only way to answer these questions is by trying stuff out. Even very experienced people make lousy decisions trying to think it through in their heads. The way I see it, C was designed by a process of mutual evolution with the Unix kernel, pre-1.0 Rust was designed via a process of mutual evolution with Servo, and maybe Modular WGSL can be designed by a co-evolution process with Bevy.

@jimblandy jimblandy closed this Jun 21, 2024
@stefnotch
Copy link
Contributor Author

@jimblandy Sorry for the late response. First of all, thank you for the really good feedback. I didn't even think about the spans remapping, that's definitely a tough problem.

I would love to have a branch on this repository for collaboration and experiments. Now that I have actually tried out my code changes in a fork of naga_oil, I very much see all of the above mentioned limitations of this design (and some more). So a iterative "try out things and communicate" approach sounds like an excellent idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

naga - compile with context
2 participants