Skip to content

Commit

Permalink
[shaders] Option to force read_write access mode on all storage bindings
Browse files Browse the repository at this point in the history
Introduced the `force_rw_storage` feature which applies a transformation
that converts all read-only storage bindings to have the `read_write`
access mode.

This is being provided as an optional preprocessor transformation to
work around the WebGPU standard's limitation on mixed `storage-read` and
`storage` usages by a GPU program over the same buffer object (see
https://www.w3.org/TR/webgpu/#programming-model-resource-usages).

This limitation makes the WGSL shaders incompatible with (Skia)
Graphite's sub-allocating buffer manager when they are run on its Dawn
backend. This workaround will be removed once Graphite's buffer
management system supports whole buffer bindings.
  • Loading branch information
armansito committed Sep 19, 2023
1 parent d4192ec commit 081c2ae
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 5 deletions.
11 changes: 11 additions & 0 deletions crates/shaders/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,17 @@ repository.workspace = true
[features]
default = ["compile", "wgsl", "msl"]
compile = ["naga", "thiserror"]

# Enabling this feature applies a transformation that converts all storage bindings
# to have the `read_write` access mode. For WGSL shaders, this affects the bind group
# layout of all pipelines and changes the usage scope of storage buffers. For MSL shaders,
# this removes the `const` qualifier from entry-point parameters in the `device` address
# space.
#
# Enabling this feature may have a performance impact and is not recommended.
force_rw_storage = []

# Target shading language variants of the vello shaders to link into the library.
wgsl = []
msl = []

Expand Down
31 changes: 26 additions & 5 deletions crates/shaders/src/compile/preprocess.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ pub fn preprocess(

match directive {
if_item @ ("ifdef" | "ifndef" | "else" | "endif") if !directive_is_at_start => {
eprintln!("#{if_item} directives must be the first non_whitespace items on their line, ignoring (line {line_number})");
eprintln!(
"#{if_item} directives must be the first non_whitespace items on \
their line, ignoring (line {line_number})"
);
break;
}
def_test @ ("ifdef" | "ifndef") => {
Expand All @@ -87,15 +90,21 @@ pub fn preprocess(
let item = stack.last_mut();
if let Some(item) = item {
if item.else_passed {
eprintln!("Second else for same ifdef/ifndef (line {line_number}); ignoring second else")
eprintln!(
"Second else for same ifdef/ifndef (line {line_number}); \
ignoring second else"
)
} else {
item.else_passed = true;
item.active = !item.active;
}
}
let remainder = directive_start[directive_len..].trim();
if !remainder.is_empty() {
eprintln!("#else directives don't take an argument. `{remainder}` will not be in output (line {line_number})");
eprintln!(
"#else directives don't take an argument. `{remainder}` will not \
be in output (line {line_number})"
);
}
// Don't add this line to the output; it should be empty (see warning above)
continue 'all_lines;
Expand All @@ -106,7 +115,10 @@ pub fn preprocess(
}
let remainder = directive_start[directive_len..].trim();
if !remainder.is_empty() {
eprintln!("#endif directives don't take an argument. `{remainder}` will not be in output (line {line_number})");
eprintln!(
"#endif directives don't take an argument. `{remainder}` will \
not be in output (line {line_number})"
);
}
// Don't add this line to the output; it should be empty (see warning above)
continue 'all_lines;
Expand All @@ -132,7 +144,8 @@ pub fn preprocess(
let import = imports.get(import_name);
if let Some(import) = import {
// In theory, we can cache this until the top item of the stack changes
// However, in practise there will only ever be at most 2 stack items, so it's reasonable to just recompute it every time
// However, in practise there will only ever be at most 2 stack items, so
// it's reasonable to just recompute it every time
if stack.iter().all(|item| item.active) {
output.push_str(&preprocess(import, defines, imports));
}
Expand All @@ -153,6 +166,14 @@ pub fn preprocess(
if line.starts_with("let ") {
output.push_str("const");
output.push_str(&line[3..]);
} else if let Some(idx) = line.find("var<storage>") {
if cfg!(feature = "force_rw_storage") {
let mut line = line.to_string();
line.replace_range(idx..12, "var<storage, read_write>");
output.push_str(&line);
} else {
output.push_str(line);
}
} else {
output.push_str(line);
}
Expand Down

0 comments on commit 081c2ae

Please sign in to comment.