Skip to content

Instantly share code, notes, and snippets.

@ncthbrt
Last active September 2, 2024 18:33
Show Gist options
  • Save ncthbrt/9ba127e7b583650eb336e3a6b6ad5208 to your computer and use it in GitHub Desktop.
Save ncthbrt/9ba127e7b583650eb336e3a6b6ad5208 to your computer and use it in GitHub Desktop.

Module System

New keyword mod allows declaration of a sub-module. This allows discovery of modules within a crate as well as providing a single entry point for processing preprocessor directives. Modules follow rust rules such that if a crate is called x which has a sub-module a, the canonical name for the module will be x::a. x::a is added to x's symbol scope. Additional keywords self, super, and crate allow users to refer to modules relative to the current module.

Future work

  • Allowing declaration of sub module code inline to the parent module
  • Rust macro that creates a set of shader modules embedded in the code based off of rust/cargo file resolution logic. These would also include the encased shader types in the rust code if no preprocessor directives would influence the result.
  • Allow modules in the same crate to mutually import symbols from one another. This will require parsing wgsl to generate the headers.
  • Later: Extension to the language to allow for definition of module and struct "shapes" and module constructors that take in modules as arguments and return a new module based off of these arguments.
  • Generic functions

Visibility

Introduction of the pub keyword that can be applied to modules, functions and other root elements to control access to each element. Visibility is first tested on the module level and then the element level. Glsl modules expose everything as they do not have support for advanced naga oil syntax. Only public entrypoints declared in the main module are exposed by the system, with the rest being demoted to a function.

Future work

  • Addition of the other visibility modifiers present in Rust
  • Support type aliasing

Imports/Rexports

The import syntax has been changed to be use <elements>.

Rexports from a module can be done by specifying pub use. Reexports don't change the symbol name in the generated code and mainly serves to better control visibility. In addition to the new use syntax, wildcard imports have been added. These follow the rust syntax and bring all exported symbols into the current module scope.

Future work

  • Scoped imports to be closer to the rust syntax

Extensions

The extend keyword will soon be added to the mvp branch. This essentially will bring all the referenced module's symbols into the current scope but unlike use, copies/ renames the symbols to be relative to the current module. Extend acts recursively so will also apply to submodules. Extend can also apply to functions provided the base function has been declared virtual and the module the function comes from has been extended in the current module. The base function can still be called using a normal use statement. The extend keyword is the only way to expose an entrypoint from another module.

Patchsets

The patch and use patchset expressions have been added to the language and replace the current function overrides. The primary difference (other than the keyword change to avoid clashes with override constants), is that patches are hygienic in the sense that they only apply to their particular subset of the import tree and respect visibility rules. This allows them to be better language citizens.

@ncthbrt
Copy link
Author

ncthbrt commented Aug 27, 2024

@robtfm I understand your concerns but also would like to keep the behaviour unsurprising to users: It's quite unusual to have public by default. And in general I've been trying to keep the behaviour close to rust.

I also looked through some of the bevy shaders and while it's true that a lot should be mostly public, not sure that's the case for the broader ecosystem. Consider a noise library. A lot of the time, there'll be a fair amount of functions there that are irrelevant to users.

Regarding plain wgsl files, I think a new extension would make sense for the extended syntax. The plain files would behave as expected then automatically.

@ncthbrt
Copy link
Author

ncthbrt commented Aug 27, 2024

neato, kind of like generics for a whole module rather than just one fn or struct.. if I understand correctly.

What are some of the preprocessor substitutions you're hoping to replace?

(I've been asked about things like loop unrolling, operator fusion. But I think that's a separate kind of flexibility)

Stuff like struct padding or implementations of a bunch of related functionality at a particular float precision (by e.g. accepting the f16 math module)

Another example is accepting a polyfill vs native hardware module to do a certain unit of work.

@robtfm
Copy link

robtfm commented Aug 27, 2024

A lot of the time, there'll be a fair amount of functions there that are irrelevant to users.

sure, but it would be bad for register pressure if they wanted one of them and had to reimplement it. generally people use gpu for performance, we shouldn't be relaxed about duplicating code. people will end up taking the library and pasting it in directly to avoid the perf hit, which is exactly what we want to avoid. i'd say a noise library (or any library) should expose everything, or use other libraries (with public apis) for building blocks.

@stefnotch
Copy link

stefnotch commented Aug 27, 2024

  1. Import all modules, and then aggressively do dead-code elimination.

After some more thinking, I don't think that this would be fully feasible. So according to the spec, a extension can result in

  • Removal of restrictions in the current specification or in previously published extensions.

e.g unrestricted_pointer_parameters

So if I only import my_shaders::bloom::cute_function, then I do not immediately know which extensions are needed. I could find out by

  • attempting to compile it and guessing based on the error message
  • inspect the cute_function and realise that I might have to do full WGSL type checking and everything to figure out what it needs (unreasonably complex)
  • require the cute_function to declare what extensions it depends on. (too verbose)
  • require the file that contains cute_function to declare what extensions could be needed. Rather sledgehammer-y, because not every function and not every struct will need all the extensions. But that's the best option that I could think of.
    (Other than the #[cfg(some extension)])

@stefnotch
Copy link

A lot of the time, there'll be a fair amount of functions there that are irrelevant to users.

sure, but it would be bad for register pressure if they wanted one of them and had to reimplement it. generally people use gpu for performance, we shouldn't be relaxed about duplicating code. people will end up taking the library and pasting it in directly to avoid the perf hit, which is exactly what we want to avoid. i'd say a noise library (or any library) should expose everything, or use other libraries (with public apis) for building blocks.

Could a compiler reasonably get rid of duplicated functions? As in,

  1. Take the function's parameters, return value, and the syntax tree of the function body's syntax tree
  2. Hash that.
  3. If there are duplicates, rewrite the output to only have one such function.

@tychedelia
Copy link

tychedelia commented Aug 27, 2024

Just a note that override is already a wgsl language feature, so we'll unfortunately need to pick a different keyword either way.

Also as a general note, I do think that sticking as close to Rust as possible should be our guiding principle, but I also think it's worth noting the ways in which shader programming is distinct. Public/private defaults are a good example, but also just generally the size of programs as well. Part of the why private by default makes sense is to promote best practices when you're operating in a 100k+ line codebase, same with any other implementation hiding feature, polymorphism, etc. I'm definitely still curious about any/all features, but the costs are just different in a 1k line shader than a big "normal" codebase, and should be taken into consideration.

Thank you for all your work here, I am very excited how this is proceeding.

@stefnotch
Copy link

stefnotch commented Aug 27, 2024

After thinking about this a bit more: Maybe we really should do the following

  • all entry points, pipeline overridable constants, etc need to be exposed in the lib.wgsl. Everything that isn't exposed in the lib.wgsl doesn't need to be accessible. So those entry points get dropped, pipeline overridable constants get demoted if possible, etc.
  • And directives are most similar to compiler flags. So a module could reasonably write code like
fn clamp_f32(...) {}
fn clamp_i32(...) { }

// Only if f16 support is enabled
#[cfg(f16)]
fn clamp_f16(...) { }

Or if a module requires f16 for basic functionality, it aggressively does a enable f16 in the lib.wgsl file. Anyone who imports it now has to also enable f16. (Or only conditionally import that module.)

I really like the second idea. I've considered the first, but I'm a bit nervous about it because it puts the onus on the user to know what to expose to the entry point. I guess if we combined that with extends, it'd mostly work out.

Ah, I finally understand what you meant with "second idea".

I suppose that a material shader that a Bevy user would write is very different from a typical shader. The material shader is way more structured and mostly behaves like "default material, but with a tiny bit of custom code"?
So I think it'd be alright to treat those as "needs extends".

But maybe preserving entry points, bindings, etc is still better?

@stefnotch
Copy link

Just a note that override is already a wgsl language feature, so we'll unfortunately need to pick a different keyword either way.

I don't believe that we would need to. We only would need to pick a new keyword if parsing were ambiguous.

override fn is currently not valid WGSL. So parsing should be unambiguous.

@tychedelia
Copy link

override fn is currently not valid WGSL. So parsing should be unambiguous.

My concern was less about parsing and more about shadowing an existing feature, but it's both a good word and obscure existing feature, so I could be easily convinced.

@ncthbrt
Copy link
Author

ncthbrt commented Aug 28, 2024

@stefnotch Ah. Damn. That's tricky.

This makes sense to me though:

  • require the file that contains cute_function to declare what extensions could be needed. Rather sledgehammer-y, because not every function and not every struct will need all the extensions. But that's the best option that I could think of.

I think I like the thought of #cfg[some_extension] more though as it can be a bit more forensic.

@stefnotch
Copy link

override fn is currently not valid WGSL. So parsing should be unambiguous.

My concern was less about parsing and more about shadowing an existing feature, but it's both a good word and obscure existing feature, so I could be easily convinced.

Very reasonable concern. Currently, it wouldn't conflict with the existing feature. (It would just look slightly odd to have the same keyword used in two different contexts.)

@stefnotch
Copy link

stefnotch commented Aug 28, 2024

@stefnotch Ah. Damn. That's tricky.

This makes sense to me though:

  • require the file that contains cute_function to declare what extensions could be needed. Rather sledgehammer-y, because not every function and not every struct will need all the extensions. But that's the best option that I could think of.

I think I like the thought of #cfg[some_extension] more though as it can be a bit more forensic.

I think I also like the cfg option the most. It's also reasonably straightforward in terms of semantics and implementation.

However, it does mean that the importing proposal is going to be "unfinished" without conditional compilation.

@stefnotch
Copy link

stefnotch commented Aug 28, 2024

I am currently looking into the Rust use semantics. After all, I might want to implement it.

My current notes are

  • The use ::something syntax also needs to be supported, otherwise one runs into situations where it is impossible to import another library. (name clash)
  • On a website, if I write use bevy_pbr::lighting::something::my_function, then I have to do 4 fetch requests. As in, I first have to load bevy_pbr.wgsl, and only then can I load bevy_pbr/lighting.wgsl, .... This is not too bad, as it is just $O(\log n)$ where $n$ is the number of modules. And usually some other part of my shader will also need bevy_pbr::....
  • On a website, we will have to implement "lazy loading"! As in, if we're importing things at runtime, then aggressively following mod statements would be too expensive.
    • This does mean that side-effects (entry points) need to be declared in the top level module. Otherwise there's no way of doing lazy loading. ❗
  • A re-export can absolutely create a cycle. It's trivial to make use foo::bar::foo::bar::foo::bar::foo....::bar; a valid import statement. Even if it's slightly nonsensical.
    • It is also possible to create self-referential re-exports. Create a file called cat.rs and write pub use crate::cat as cyclicat;
  • Wildcards are compatible with cycles. I haven't fully thought through how identifier resolution works with that, and shadowing.
  • Wildcards can lead to ambiguities. I don't like that. Simply adding a function to a shader should not be able to break other shaders.
  • The Rust reference on imports says "incomplete". :(

@ncthbrt
Copy link
Author

ncthbrt commented Aug 28, 2024

@robtfm @tychedelia would be willing (if still not fully convinced) to explore public by default. However I'd like to wait for post POC as a lot of assumptions are baked into the code regarding visibility and want to get something to show people

@ncthbrt
Copy link
Author

ncthbrt commented Aug 28, 2024

On a website, we will have to implement "lazy loading"! As in, if we're importing things at runtime, then aggressively following mod statements would be too expensive.

Would it help matters if we considered the idea of a module being open/closed? As in bevy::lighting::foo would open two modules and usage of foo::bar would open the third?

@stefnotch
Copy link

On a website, we will have to implement "lazy loading"! As in, if we're importing things at runtime, then aggressively following mod statements would be too expensive.

Would it help matters if we considered the idea of a module being open/closed? As in bevy::lighting::foo would open two modules and usage of foo::bar would open the third?

I think so, that's probably the most sensible way of describing and implementing it.

@ncthbrt
Copy link
Author

ncthbrt commented Aug 28, 2024

override fn is currently not valid WGSL. So parsing should be unambiguous.

My concern was less about parsing and more about shadowing an existing feature, but it's both a good word and obscure existing feature, so I could be easily convinced.

Very reasonable concern. Currently, it wouldn't conflict with the existing feature. (It would just look slightly odd to have the same keyword used in two different contexts.)

As mentioned above I think we could and should remove virtual functions in favour of extends. Which makes the point somewhat moot (unless we use override fn instead of extends fn

@stefnotch
Copy link

sure, but it would be bad for register pressure if they wanted one of them and had to reimplement it. generally people use gpu for performance, we shouldn't be relaxed about duplicating code.

After looking a bit at shader compilation, I am no longer sure if there is even any overhead to duplicating a function. The generated SPIR-V is longer, but after the driver does its compilation, it's a whole different game.

I could look more into it, but if anyone knows a person who understands that, then please do reach out to them.

@robtfm
Copy link

robtfm commented Aug 29, 2024

some current compilers don’t even convert x/2.0+1.0 into an fma, let alone do (potentially expensive) function eq tests to eliminate duplicates.

@stefnotch
Copy link

I don't think that a shader compiler would do function equality testing, but instead it would do aggressive inlining, at which point it doesn't matter which function the code came from. But I have no idea what state of the art shader compilers do, so I might be completely wrong here.

@robtfm
Copy link

robtfm commented Aug 29, 2024

again not something you can rely on. inlining may not apply for functions called in many places since the register cost of inlining can cause less waves to fit on the gpu (the same problem we would get from duplicating the function in the first place).

@stefnotch
Copy link

Thanks for the insights, good to know that there is actually a cost to useless function duplicates.

@stefnotch
Copy link

stefnotch commented Aug 30, 2024

I figured I'd note down what all the significant differences between this and the WIP import proposal are. Does this cover everything, or are there significant parts that I missed?

Things that assume a world with mod are marked with a 🐈

  • mod keyword for discovering modules 🐈
    • Strictly tree-like folder structure. Without re-exports foo::bar::baz means that there is a foo.wesl, a foo/bar.wesl and a foo/bar/baz.wesl
    • (Good language servers will still scan for files to find all root modules and report all errors)
  • Re-exporting modules 🐈
    • Can add more valid qualified paths for a module.
    • Causes path resolution to require looking at each module of the path
  • Re-exporting items
    • Can add more valid paths to an item
    • Causes name resolution to require looking at multiple modules to find the source
  • Qualified paths, like foo::bar::baz being valid in the source code 🐈
    • Easily unambiguously refer to an item, without having to think up yet another alias.
    • Any part of the code can add a dependency on another module.
  • Wildcards
    • Wildcard importing items, which will need shadowing and checking for ambiguity use foo::*;
    • Wildcard exporting items, which will need checking for ambiguity pub use bar::*
  • Visibility
    • Can be applied to modules 🐈
    • Can be applied to items
  • Extensions
  • Patchsets

@ncthbrt
Copy link
Author

ncthbrt commented Aug 30, 2024

Does this cover everything, or are there significant parts that I missed?

Think that's a succinct summary of the differences beyond superficial syntax

@ncthbrt
Copy link
Author

ncthbrt commented Aug 30, 2024

@stefnotch another difference is the use of a different path character (::) to refer to a member of a module, making it unambiguous as to whether you're accessing a member of a module vs a field in a struct

@stefnotch
Copy link

Right, that is a good point.
cat::paws definitely refers to a module and we know that it does right after we finished parsing.
weasel.paws might refer to a module, and we first have to run name resolution on it. After name resolution using just the current file, it is unambiguous.

@stefnotch
Copy link

Regarding "private by default", we could also have an "unsafe" bypass for it. As in, if I really want to access a private shader function, then I bypass the safeguard and use it. And if the library author changes or removes it, that's on me.

@ncthbrt
Copy link
Author

ncthbrt commented Sep 2, 2024

Interesting idea @stefnotch!

@stefnotch
Copy link

Right, that is a good point. cat::paws definitely refers to a module and we know that it does right after we finished parsing. weasel.paws might refer to a module, and we first have to run name resolution on it. After name resolution using just the current file, it is unambiguous.

Actually, this is slightly more complicated. It's known that cat::paws refers to a module, but one cannot figure out which module until after name resolution. Wildcard imports make this especially complex, and mean that one needs name resolution with multiple files.

weasel.paws could either be an importable item from a module, or a property access. Also needs name resolution to figure this out. The WIP proposal only needs the current file for name resolution to find the correct module.

@stefnotch
Copy link

stefnotch commented Sep 2, 2024

Reposting suggestion regarding side effects from Discord

Side effects:

  • Things that are specified when creating a WGSL pipeline
  • Directives: Behind cfg flags, root module can specify required directives
  • (Maybe const_assert?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment