From 081c2ae3c7f9ce7c3a5b17fda96ad613e0c4996a Mon Sep 17 00:00:00 2001 From: Arman Uguray Date: Mon, 18 Sep 2023 19:21:58 -0700 Subject: [PATCH] [shaders] Option to force read_write access mode on all storage bindings 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. --- crates/shaders/Cargo.toml | 11 +++++++++ crates/shaders/src/compile/preprocess.rs | 31 ++++++++++++++++++++---- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/crates/shaders/Cargo.toml b/crates/shaders/Cargo.toml index bb534f956..e6f8c739b 100644 --- a/crates/shaders/Cargo.toml +++ b/crates/shaders/Cargo.toml @@ -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 = [] diff --git a/crates/shaders/src/compile/preprocess.rs b/crates/shaders/src/compile/preprocess.rs index 199ea4073..0b43ebfb2 100644 --- a/crates/shaders/src/compile/preprocess.rs +++ b/crates/shaders/src/compile/preprocess.rs @@ -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") => { @@ -87,7 +90,10 @@ 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; @@ -95,7 +101,10 @@ pub fn preprocess( } 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; @@ -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; @@ -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)); } @@ -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") { + if cfg!(feature = "force_rw_storage") { + let mut line = line.to_string(); + line.replace_range(idx..12, "var"); + output.push_str(&line); + } else { + output.push_str(line); + } } else { output.push_str(line); }