From 25d133f972cc218c43d247f5d590e3e30146a9b3 Mon Sep 17 00:00:00 2001 From: stefnotch Date: Fri, 7 Jun 2024 14:48:32 +0200 Subject: [PATCH 01/11] Design public API --- naga/src/front/wgsl/mod.rs | 54 +++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 7 deletions(-) diff --git a/naga/src/front/wgsl/mod.rs b/naga/src/front/wgsl/mod.rs index aec1e657fc..34293c503c 100644 --- a/naga/src/front/wgsl/mod.rs +++ b/naga/src/front/wgsl/mod.rs @@ -18,7 +18,9 @@ use thiserror::Error; pub use crate::front::wgsl::error::ParseError; use crate::front::wgsl::lower::Lowerer; -use crate::Scalar; +use crate::{Arena, FastHashMap, Scalar}; + +use self::parse::ast::TranslationUnit; pub struct Frontend { parser: Parser, @@ -32,18 +34,56 @@ impl Frontend { } pub fn parse(&mut self, source: &str) -> Result { - self.inner(source).map_err(|x| x.as_parse_error(source)) + self.inner_to_ast(source) + .map_err(|x| x.as_parse_error(source)) + .and_then(|v| v.to_module()) } - fn inner<'a>(&mut self, source: &'a str) -> Result> { - let tu = self.parser.parse(source)?; - let index = index::Index::generate(&tu)?; - let module = Lowerer::new(&index).lower(&tu)?; + /// Two-step module conversion, can be used to modify the intermediate structure before it becomes a module. + pub fn parse_to_ast<'a>(&mut self, source: &'a str) -> Result, ParseError> { + self.inner_to_ast(source) + .map_err(|x| x.as_parse_error(source)) + } - Ok(module) + fn inner_to_ast<'a>(&mut self, source: &'a str) -> Result, Error<'a>> { + let mut result = ParsedWgsl { + source, + translation_unit: self.parser.parse(source)?, + index: index::Index::empty(), + globals: Default::default(), + }; + result.index = index::Index::generate(&result.translation_unit)?; + Ok(result) } } +pub enum ProvidedGlobalDecl { + Function(crate::Function), + Var(crate::GlobalVariable), + Const(crate::Constant), + Override(crate::Override), + Type(crate::Type), + EntryPoint(crate::EntryPoint), +} + +pub struct ParsedWgsl<'a> { + source: &'a str, + translation_unit: TranslationUnit<'a>, + index: index::Index<'a>, + // global_names: Arena, + globals: FastHashMap, +} +impl<'a> ParsedWgsl<'a> { + pub fn to_module(&self) -> Result { + Lowerer::new(&self.index) + .lower(&self.translation_unit, &self.globals) + .map_err(|x| x.as_parse_error(self.source)) + } + + pub fn add_global(&mut self, name: String, value: ProvidedGlobalDecl) { + self.globals.insert(name, value); + } +} ///
// NOTE: Keep this in sync with `wgpu::Device::create_shader_module`! // NOTE: Keep this in sync with `wgpu_core::Global::device_create_shader_module`! From 49ccd6c200846a904b79a4e4409a1b9caa8f92b9 Mon Sep 17 00:00:00 2001 From: Stefnotch Date: Fri, 7 Jun 2024 23:31:13 +0200 Subject: [PATCH 02/11] Potential create-module-with-context implementation --- naga/src/front/wgsl/lower/mod.rs | 40 ++++++++++++++++++++++++- naga/src/front/wgsl/mod.rs | 50 +++++++++++--------------------- 2 files changed, 56 insertions(+), 34 deletions(-) diff --git a/naga/src/front/wgsl/lower/mod.rs b/naga/src/front/wgsl/lower/mod.rs index e7cce17723..9df3ae82dc 100644 --- a/naga/src/front/wgsl/lower/mod.rs +++ b/naga/src/front/wgsl/lower/mod.rs @@ -913,8 +913,9 @@ impl<'source, 'temp> Lowerer<'source, 'temp> { pub fn lower( &mut self, tu: &'temp ast::TranslationUnit<'source>, + base_module: Option<&'source crate::Module>, ) -> Result> { - let mut module = crate::Module::default(); + let mut module = base_module.map(|v| v.to_owned()).unwrap_or_default(); let mut ctx = GlobalContext { ast_expressions: &tu.expressions, @@ -925,6 +926,43 @@ impl<'source, 'temp> Lowerer<'source, 'temp> { global_expression_kind_tracker: &mut crate::proc::ExpressionKindTracker::new(), }; + if let Some(base_module) = base_module { + // The handles for base_module are equal to the handles for ctx.module, because we just cloned the arenas. + for (handle, f) in base_module.functions.iter() { + if let Some(name) = f.name.as_ref() { + ctx.globals + .insert(name, LoweredGlobalDecl::Function(handle)); + } + } + for (handle, v) in base_module.global_variables.iter() { + if let Some(name) = v.name.as_ref() { + ctx.globals.insert(name, LoweredGlobalDecl::Var(handle)); + } + } + for (handle, c) in base_module.constants.iter() { + if let Some(name) = c.name.as_ref() { + ctx.globals.insert(name, LoweredGlobalDecl::Const(handle)); + } + } + for (handle, o) in base_module.overrides.iter() { + if let Some(name) = o.name.as_ref() { + ctx.globals + .insert(name, LoweredGlobalDecl::Override(handle)); + } + } + for (handle, t) in base_module.types.iter() { + if let Some(name) = t.name.as_ref() { + ctx.globals.insert(name, LoweredGlobalDecl::Type(handle)); + } + } + for entry_point in base_module.entry_points.iter() { + ctx.globals + .insert(entry_point.name.as_str(), LoweredGlobalDecl::EntryPoint); + } + *ctx.global_expression_kind_tracker = + crate::proc::ExpressionKindTracker::from_arena(&ctx.module.global_expressions); + } + for decl_handle in self.index.visit_ordered() { let span = tu.decls.get_span(decl_handle); let decl = &tu.decls[decl_handle]; diff --git a/naga/src/front/wgsl/mod.rs b/naga/src/front/wgsl/mod.rs index 34293c503c..3a937f45ff 100644 --- a/naga/src/front/wgsl/mod.rs +++ b/naga/src/front/wgsl/mod.rs @@ -18,7 +18,7 @@ use thiserror::Error; pub use crate::front::wgsl::error::ParseError; use crate::front::wgsl::lower::Lowerer; -use crate::{Arena, FastHashMap, Scalar}; +use crate::Scalar; use self::parse::ast::TranslationUnit; @@ -34,55 +34,39 @@ impl Frontend { } pub fn parse(&mut self, source: &str) -> Result { - self.inner_to_ast(source) - .map_err(|x| x.as_parse_error(source)) - .and_then(|v| v.to_module()) + self.parse_to_ast(source).and_then(|v| v.to_module(None)) } /// Two-step module conversion, can be used to modify the intermediate structure before it becomes a module. pub fn parse_to_ast<'a>(&mut self, source: &'a str) -> Result, ParseError> { - self.inner_to_ast(source) - .map_err(|x| x.as_parse_error(source)) - } - - fn inner_to_ast<'a>(&mut self, source: &'a str) -> Result, Error<'a>> { - let mut result = ParsedWgsl { + let translation_unit = self + .parser + .parse(source) + .map_err(|x| x.as_parse_error(source))?; + let index = + index::Index::generate(&translation_unit).map_err(|x| x.as_parse_error(source))?; + Ok(ParsedWgsl { source, - translation_unit: self.parser.parse(source)?, - index: index::Index::empty(), - globals: Default::default(), - }; - result.index = index::Index::generate(&result.translation_unit)?; - Ok(result) + translation_unit, + index, + }) } } -pub enum ProvidedGlobalDecl { - Function(crate::Function), - Var(crate::GlobalVariable), - Const(crate::Constant), - Override(crate::Override), - Type(crate::Type), - EntryPoint(crate::EntryPoint), -} - pub struct ParsedWgsl<'a> { source: &'a str, translation_unit: TranslationUnit<'a>, index: index::Index<'a>, - // global_names: Arena, - globals: FastHashMap, } impl<'a> ParsedWgsl<'a> { - pub fn to_module(&self) -> Result { + pub fn to_module( + &self, + base_module: Option<&crate::Module>, + ) -> Result { Lowerer::new(&self.index) - .lower(&self.translation_unit, &self.globals) + .lower(&self.translation_unit, base_module) .map_err(|x| x.as_parse_error(self.source)) } - - pub fn add_global(&mut self, name: String, value: ProvidedGlobalDecl) { - self.globals.insert(name, value); - } } ///
// NOTE: Keep this in sync with `wgpu::Device::create_shader_module`! From cb35f8e040785401ff65ecf37605be23571fc2c2 Mon Sep 17 00:00:00 2001 From: Stefnotch Date: Sat, 8 Jun 2024 10:54:20 +0200 Subject: [PATCH 03/11] Re-add private method for testing --- naga/src/front/wgsl/mod.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/naga/src/front/wgsl/mod.rs b/naga/src/front/wgsl/mod.rs index 3a937f45ff..7edb09d71e 100644 --- a/naga/src/front/wgsl/mod.rs +++ b/naga/src/front/wgsl/mod.rs @@ -51,6 +51,13 @@ impl Frontend { index, }) } + + fn inner<'a>(&mut self, source: &'a str) -> Result> { + let tu = self.parser.parse(source)?; + let index = index::Index::generate(&tu)?; + let module = Lowerer::new(&index).lower(&tu, None)?; + Ok(module) + } } pub struct ParsedWgsl<'a> { From 4b80af7faae503deed89fa1d62ff086ec9356d37 Mon Sep 17 00:00:00 2001 From: Stefnotch Date: Sun, 9 Jun 2024 14:28:36 +0200 Subject: [PATCH 04/11] Add majority of tests for base module API --- naga/src/front/wgsl/tests.rs | 146 +++++++++++++++++++++++++++++++++++ 1 file changed, 146 insertions(+) diff --git a/naga/src/front/wgsl/tests.rs b/naga/src/front/wgsl/tests.rs index cc3d858317..98d4684911 100644 --- a/naga/src/front/wgsl/tests.rs +++ b/naga/src/front/wgsl/tests.rs @@ -636,3 +636,149 @@ fn parse_missing_workgroup_size() { Error::MissingWorkgroupSize(span) if span == Span::new(1, 8) )); } + +#[test] +fn parse_base_module_constant() { + use crate::front::wgsl::Frontend; + + let base_module = parse_str("const some_constant_value: f32 = 1.0;").unwrap(); + let shader = "fn foo() -> vec4 { return vec4(some_constant_value); }"; + Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); +} + +#[test] +fn parse_structs_with_base_module_structs() { + use crate::front::wgsl::Frontend; + + let base_module = parse_str("struct Bar { a: vec4f, b: Foo }; struct Foo { c: f32 }").unwrap(); + let shader = "fn foo(foo: Foo) -> Bar { return Bar(vec4(1.0), foo); }"; + Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); +} + +#[test] +fn parse_fn_with_base_module() { + use crate::front::wgsl::Frontend; + + let base_module = + parse_str("fn cat() -> Foo { return Foo(1.0); }; struct Foo { c: f32 }").unwrap(); + let shader = "fn foo() -> f32 { return cat().c; }"; + Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); +} + +#[test] +fn parse_fn_conflict_with_base_module() { + use crate::front::wgsl::Frontend; + + // TODO: Error::Redefinition should be triggered + let base_module = parse_str("fn cat() -> f32 { return 1.0; }").unwrap(); + let shader = "fn cat() -> f32 { return 2.0; }"; + let result = Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)); + assert!(result.is_err()); +} + +#[test] +fn parse_base_module_alias() { + use crate::front::wgsl::Frontend; + + let base_module = + parse_str("alias number = u32; struct Bar { a: number, b: Foo }; alias Foo = i32;") + .unwrap(); + let shader = "fn foo(a: u32) -> Bar { return Bar(a, -1i); }"; + Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); +} + +#[test] +fn parse_base_module_alias_usage() { + use crate::front::wgsl::Frontend; + + let base_module = parse_str("alias number = u32; struct Bar { a: u32 };").unwrap(); + let shader = "fn foo(a: number) -> Bar { return Bar(a); }"; + Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); +} + +/* Add this once https://github.com/gfx-rs/wgpu/issues/5786 is fixed +#[test] +fn parse_base_module_alias_predeclared() { + use crate::front::wgsl::Frontend; + + let base_module = + parse_str("alias vec4f = u32; struct Bar { a: vec4f, b: Foo }; alias Foo = i32;").unwrap(); + let shader = "fn foo(a: u32) -> Bar { return Bar(a, -1i); }"; + Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); +} + */ + +#[test] +fn parse_base_module_function_predefined() { + use crate::front::wgsl::Frontend; + + // Changing a predefined function should affect the shader + let base_module = parse_str( + "fn bar(a: f32) -> f32 { return cos(vec3f(a)).z; } fn cos(a: vec3f) -> vec3f { return a.xyz; }", + ) + .unwrap(); + let shader = "fn foo(a: f32) -> f32 { return cos(1.0); }"; + Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); +} + +#[test] +fn parse_base_module_function_predefined_no_leak() { + use crate::front::wgsl::Frontend; + + // But changing a predefined function in the shader should not affect the base module + let base_module = parse_str("fn bar(a: f32) -> f32 { return cos(a); }").unwrap(); + let shader = + "fn foo(a: f32) -> f32 { return cos(vec3f(1.0)).z; } fn cos(a: vec3f) -> vec3f { return a.xyz; }"; + Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); +} + +// Use const as a const and as a runtime value in the actual module. Or use const with different types (abstract number type). + +// Two levels of base modules + +// Const in base module with same name as a variable in the actual module + +// Entry point in both + +// Overrides in both + +// @group(0) @binding(0) +// var foo: array; in both + +// Error: redefining a global in the actual module. What does the message say? +// Error reusing a group-binding pair in the actual module. What does the message say? From fe2cb2dcf447eddc11ba5a39c9d2c8fda3adf3e2 Mon Sep 17 00:00:00 2001 From: Stefnotch Date: Sun, 9 Jun 2024 22:48:28 +0200 Subject: [PATCH 05/11] Add unit tests --- naga/src/front/wgsl/tests.rs | 134 ++++++++++++++++++++++++++++++++--- 1 file changed, 124 insertions(+), 10 deletions(-) diff --git a/naga/src/front/wgsl/tests.rs b/naga/src/front/wgsl/tests.rs index 98d4684911..34b337bbcb 100644 --- a/naga/src/front/wgsl/tests.rs +++ b/naga/src/front/wgsl/tests.rs @@ -650,6 +650,23 @@ fn parse_base_module_constant() { .unwrap(); } +/* Add this test once abstract numerics are supported. +Although, depending on the implementation, this may not work anyways. +#[test] +fn parse_base_module_abstract_numerics() { + use crate::front::wgsl::Frontend; + + let base_module = + parse_str("const two = 2;") + .unwrap(); + let shader = "const four: u32 = 2 * two; const signed_two: i32 = two;"; + Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); +} +*/ #[test] fn parse_structs_with_base_module_structs() { use crate::front::wgsl::Frontend; @@ -759,7 +776,7 @@ fn parse_base_module_function_predefined_no_leak() { // But changing a predefined function in the shader should not affect the base module let base_module = parse_str("fn bar(a: f32) -> f32 { return cos(a); }").unwrap(); let shader = - "fn foo(a: f32) -> f32 { return cos(vec3f(1.0)).z; } fn cos(a: vec3f) -> vec3f { return a.xyz; }"; + "fn foo(a: f32) -> f32 { return cos(vec3f(a)).z + bar(a); } fn cos(a: vec3f) -> vec3f { return a.xyz; }"; Frontend::new() .parse_to_ast(shader) .unwrap() @@ -767,18 +784,115 @@ fn parse_base_module_function_predefined_no_leak() { .unwrap(); } -// Use const as a const and as a runtime value in the actual module. Or use const with different types (abstract number type). +#[test] +fn parse_base_module_twice() { + use crate::front::wgsl::Frontend; + + let base_module_a = parse_str("fn bar(a: f32) -> f32 { return cos(a); }").unwrap(); + let shader_a = "fn foo(a: f32) -> f32 { return bar(a); }"; + let base_module_b = Frontend::new() + .parse_to_ast(shader_a) + .unwrap() + .to_module(Some(&base_module_a)) + .unwrap(); + let shader_b = "fn foobar(a: f32) -> f32 { return bar(a) + foo(a); }"; + Frontend::new() + .parse_to_ast(shader_b) + .unwrap() + .to_module(Some(&base_module_b)) + .unwrap(); +} -// Two levels of base modules +#[test] +fn parse_base_module_const_conflict() { + use crate::front::wgsl::Frontend; -// Const in base module with same name as a variable in the actual module + let base_module = parse_str("const foo: f32 = 1.0;").unwrap(); + let shader = "fn foo() -> f32 { return 2.0; }"; + let result = Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)); + assert!(result.is_err()); +} -// Entry point in both +#[test] +fn parse_base_module_const_local() { + use crate::front::wgsl::Frontend; + // Const in base module with same name as a local variable in the actual module shouldn't cause conflicts -// Overrides in both + let base_module = parse_str("const foo: vec3f = vec3f(1.0);").unwrap(); + let shader = "fn foo() -> f32 { let foo: f32 = 2.0; return foo; }"; + Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); +} -// @group(0) @binding(0) -// var foo: array; in both +#[test] +fn parse_base_module_entry_points() { + use crate::front::wgsl::Frontend; -// Error: redefining a global in the actual module. What does the message say? -// Error reusing a group-binding pair in the actual module. What does the message say? + let base_module = + parse_str("@vertex fn vs() -> @builtin(position) vec4f { return vec4(0.0); }") + .unwrap(); + let shader = "@fragment fn fs() -> @location(0) vec4f { return vec4(1.0); }"; + let result = Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); + assert_eq!(result.entry_points.len(), 2); +} + +#[test] +fn parse_base_module_pipeline_overridable_constants() { + use crate::front::wgsl::Frontend; + + let base_module = parse_str("override diffuse_param: f32 = 2.3;").unwrap(); + let shader = "override specular_param: f32 = 0.1; fn foo() -> f32 { return diffuse_param + specular_param; }"; + Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); +} + +#[test] +fn parse_base_module_storage_buffers() { + use crate::front::wgsl::Frontend; + + let base_module = parse_str( + "@group(0) @binding(0) + var foo: array;", + ) + .unwrap(); + let shader = "@group(0) @binding(1) + var bar: array;"; + Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); +} + +/* Add this test once https://github.com/gfx-rs/wgpu/issues/5787 is fixed +#[test] +fn parse_base_module_storage_buffers_conflict() { + use crate::front::wgsl::Frontend; + + let base_module = parse_str( + "@group(0) @binding(0) + var foo: array;", + ) + .unwrap(); + let shader = "@group(0) @binding(0) + var bar: array;"; + let result = Frontend::new() + .parse_to_ast(shader) + .unwrap() + .to_module(Some(&base_module)); + assert!(result.is_err()); +} + */ From 62a49e3f7980f5d28b42079cc6e075357d7bad9e Mon Sep 17 00:00:00 2001 From: Stefnotch Date: Sun, 9 Jun 2024 23:04:16 +0200 Subject: [PATCH 06/11] Fix Error::Redefinition checks for base module --- naga/src/front/wgsl/lower/mod.rs | 27 +++++++++++++++++++++++++++ naga/src/front/wgsl/tests.rs | 2 +- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/naga/src/front/wgsl/lower/mod.rs b/naga/src/front/wgsl/lower/mod.rs index 9df3ae82dc..b2cc8f3385 100644 --- a/naga/src/front/wgsl/lower/mod.rs +++ b/naga/src/front/wgsl/lower/mod.rs @@ -963,6 +963,33 @@ impl<'source, 'temp> Lowerer<'source, 'temp> { crate::proc::ExpressionKindTracker::from_arena(&ctx.module.global_expressions); } + // check for redefinitions + for (_, decl) in tu.decls.iter() { + let ident = match decl.kind { + ast::GlobalDeclKind::Fn(ref f) => f.name, + ast::GlobalDeclKind::Var(ref v) => v.name, + ast::GlobalDeclKind::Const(ref c) => c.name, + ast::GlobalDeclKind::Override(ref o) => o.name, + ast::GlobalDeclKind::Struct(ref s) => s.name, + ast::GlobalDeclKind::Type(ref t) => t.name, + }; + if let Some(old) = ctx.globals.get(ident.name) { + let span = match old { + LoweredGlobalDecl::Function(handle) => ctx.module.functions.get_span(*handle), + LoweredGlobalDecl::Var(handle) => ctx.module.global_variables.get_span(*handle), + LoweredGlobalDecl::Const(handle) => ctx.module.constants.get_span(*handle), + LoweredGlobalDecl::Override(handle) => ctx.module.overrides.get_span(*handle), + LoweredGlobalDecl::Type(handle) => ctx.module.types.get_span(*handle), + // We don't have good spans for entry points + LoweredGlobalDecl::EntryPoint => Default::default(), + }; + return Err(Error::Redefinition { + previous: span, + current: ident.span, + }); + } + } + for decl_handle in self.index.visit_ordered() { let span = tu.decls.get_span(decl_handle); let decl = &tu.decls[decl_handle]; diff --git a/naga/src/front/wgsl/tests.rs b/naga/src/front/wgsl/tests.rs index 34b337bbcb..856b85d828 100644 --- a/naga/src/front/wgsl/tests.rs +++ b/naga/src/front/wgsl/tests.rs @@ -822,7 +822,7 @@ fn parse_base_module_const_local() { // Const in base module with same name as a local variable in the actual module shouldn't cause conflicts let base_module = parse_str("const foo: vec3f = vec3f(1.0);").unwrap(); - let shader = "fn foo() -> f32 { let foo: f32 = 2.0; return foo; }"; + let shader = "fn bar() -> f32 { let foo: f32 = 2.0; return foo; }"; Frontend::new() .parse_to_ast(shader) .unwrap() From c2041b1d3bc9b53c2944594d03ae78f35580b5fb Mon Sep 17 00:00:00 2001 From: Stefnotch Date: Mon, 10 Jun 2024 09:10:20 +0200 Subject: [PATCH 07/11] Check for Error::Redefinition in the tests --- naga/src/front/wgsl/lower/mod.rs | 12 ++++++------ naga/src/front/wgsl/mod.rs | 31 +++++++++++++++---------------- naga/src/front/wgsl/tests.rs | 27 ++++++++++++++------------- 3 files changed, 35 insertions(+), 35 deletions(-) diff --git a/naga/src/front/wgsl/lower/mod.rs b/naga/src/front/wgsl/lower/mod.rs index b2cc8f3385..201c104d0a 100644 --- a/naga/src/front/wgsl/lower/mod.rs +++ b/naga/src/front/wgsl/lower/mod.rs @@ -974,12 +974,12 @@ impl<'source, 'temp> Lowerer<'source, 'temp> { ast::GlobalDeclKind::Type(ref t) => t.name, }; if let Some(old) = ctx.globals.get(ident.name) { - let span = match old { - LoweredGlobalDecl::Function(handle) => ctx.module.functions.get_span(*handle), - LoweredGlobalDecl::Var(handle) => ctx.module.global_variables.get_span(*handle), - LoweredGlobalDecl::Const(handle) => ctx.module.constants.get_span(*handle), - LoweredGlobalDecl::Override(handle) => ctx.module.overrides.get_span(*handle), - LoweredGlobalDecl::Type(handle) => ctx.module.types.get_span(*handle), + let span = match *old { + LoweredGlobalDecl::Function(handle) => ctx.module.functions.get_span(handle), + LoweredGlobalDecl::Var(handle) => ctx.module.global_variables.get_span(handle), + LoweredGlobalDecl::Const(handle) => ctx.module.constants.get_span(handle), + LoweredGlobalDecl::Override(handle) => ctx.module.overrides.get_span(handle), + LoweredGlobalDecl::Type(handle) => ctx.module.types.get_span(handle), // We don't have good spans for entry points LoweredGlobalDecl::EntryPoint => Default::default(), }; diff --git a/naga/src/front/wgsl/mod.rs b/naga/src/front/wgsl/mod.rs index 7edb09d71e..7312fd0dd0 100644 --- a/naga/src/front/wgsl/mod.rs +++ b/naga/src/front/wgsl/mod.rs @@ -34,30 +34,24 @@ impl Frontend { } pub fn parse(&mut self, source: &str) -> Result { - self.parse_to_ast(source).and_then(|v| v.to_module(None)) + self.parse_to_ast(source)?.to_module(None) } /// Two-step module conversion, can be used to modify the intermediate structure before it becomes a module. pub fn parse_to_ast<'a>(&mut self, source: &'a str) -> Result, ParseError> { - let translation_unit = self - .parser - .parse(source) - .map_err(|x| x.as_parse_error(source))?; - let index = - index::Index::generate(&translation_unit).map_err(|x| x.as_parse_error(source))?; + self.inner_to_ast(source) + .map_err(|x| x.as_parse_error(source)) + } + + fn inner_to_ast<'a>(&mut self, source: &'a str) -> Result, Error<'a>> { + let translation_unit = self.parser.parse(source)?; + let index = index::Index::generate(&translation_unit)?; Ok(ParsedWgsl { source, translation_unit, index, }) } - - fn inner<'a>(&mut self, source: &'a str) -> Result> { - let tu = self.parser.parse(source)?; - let index = index::Index::generate(&tu)?; - let module = Lowerer::new(&index).lower(&tu, None)?; - Ok(module) - } } pub struct ParsedWgsl<'a> { @@ -70,10 +64,15 @@ impl<'a> ParsedWgsl<'a> { &self, base_module: Option<&crate::Module>, ) -> Result { - Lowerer::new(&self.index) - .lower(&self.translation_unit, base_module) + self.inner_to_module(base_module) .map_err(|x| x.as_parse_error(self.source)) } + fn inner_to_module( + &self, + base_module: Option<&'a crate::Module>, + ) -> Result> { + Lowerer::new(&self.index).lower(&self.translation_unit, base_module) + } } ///
// NOTE: Keep this in sync with `wgpu::Device::create_shader_module`! diff --git a/naga/src/front/wgsl/tests.rs b/naga/src/front/wgsl/tests.rs index 856b85d828..2f8ba9d67d 100644 --- a/naga/src/front/wgsl/tests.rs +++ b/naga/src/front/wgsl/tests.rs @@ -614,7 +614,9 @@ fn parse_repeated_attributes() { let span_end = span_start + name_length; let expected_span = Span::new(span_start, span_end); - let result = Frontend::new().inner(&shader); + let result = Frontend::new() + .inner_to_ast(&shader) + .and_then(|v| v.inner_to_module(None)); assert!(matches!( result.unwrap_err(), Error::RepeatedAttribute(span) if span == expected_span @@ -630,7 +632,9 @@ fn parse_missing_workgroup_size() { }; let shader = "@compute fn vs() -> vec4 { return vec4(0.0); }"; - let result = Frontend::new().inner(shader); + let result = Frontend::new() + .inner_to_ast(shader) + .and_then(|v| v.inner_to_module(None)); assert!(matches!( result.unwrap_err(), Error::MissingWorkgroupSize(span) if span == Span::new(1, 8) @@ -696,16 +700,14 @@ fn parse_fn_with_base_module() { #[test] fn parse_fn_conflict_with_base_module() { - use crate::front::wgsl::Frontend; + use crate::front::wgsl::{error::Error, Frontend}; - // TODO: Error::Redefinition should be triggered let base_module = parse_str("fn cat() -> f32 { return 1.0; }").unwrap(); let shader = "fn cat() -> f32 { return 2.0; }"; let result = Frontend::new() - .parse_to_ast(shader) - .unwrap() - .to_module(Some(&base_module)); - assert!(result.is_err()); + .inner_to_ast(shader) + .and_then(|v| v.inner_to_module(Some(&base_module))); + assert!(matches!(result, Err(Error::Redefinition { .. }))); } #[test] @@ -805,15 +807,14 @@ fn parse_base_module_twice() { #[test] fn parse_base_module_const_conflict() { - use crate::front::wgsl::Frontend; + use crate::front::wgsl::{error::Error, Frontend}; let base_module = parse_str("const foo: f32 = 1.0;").unwrap(); let shader = "fn foo() -> f32 { return 2.0; }"; let result = Frontend::new() - .parse_to_ast(shader) - .unwrap() - .to_module(Some(&base_module)); - assert!(result.is_err()); + .inner_to_ast(shader) + .and_then(|v| v.inner_to_module(Some(&base_module))); + assert!(matches!(result, Err(Error::Redefinition { .. }))); } #[test] From 46c9be645516c3e96a119aaf2ca2e24b9e339631 Mon Sep 17 00:00:00 2001 From: Stefnotch Date: Mon, 10 Jun 2024 09:15:28 +0200 Subject: [PATCH 08/11] Improve documentation --- naga/src/front/wgsl/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/naga/src/front/wgsl/mod.rs b/naga/src/front/wgsl/mod.rs index 7312fd0dd0..919d199140 100644 --- a/naga/src/front/wgsl/mod.rs +++ b/naga/src/front/wgsl/mod.rs @@ -37,7 +37,7 @@ impl Frontend { self.parse_to_ast(source)?.to_module(None) } - /// Two-step module conversion, can be used to modify the intermediate structure before it becomes a module. + /// Two-step module conversion, can be used to compile with a "base module". pub fn parse_to_ast<'a>(&mut self, source: &'a str) -> Result, ParseError> { self.inner_to_ast(source) .map_err(|x| x.as_parse_error(source)) From a6cd001e4e94d8c56a21ab3786c5d3f06ca787f1 Mon Sep 17 00:00:00 2001 From: Stefnotch Date: Mon, 10 Jun 2024 09:36:42 +0200 Subject: [PATCH 09/11] Add changelog entry --- CHANGELOG.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78075b4a06..6d5400547d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -95,6 +95,27 @@ By @stefnotch in [#5410](https://github.com/gfx-rs/wgpu/pull/5410) - Implement `WGSL`'s `unpack4xI8`,`unpack4xU8`,`pack4xI8` and `pack4xU8`. By @VlaDexa in [#5424](https://github.com/gfx-rs/wgpu/pull/5424) - Began work adding support for atomics to the SPIR-V frontend. Tracking issue is [here](https://github.com/gfx-rs/wgpu/issues/4489). By @schell in [#5702](https://github.com/gfx-rs/wgpu/pull/5702). +- 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; + + let base_module = Frontend::new() + .parse(" + fn main_image(frag_coord: vec2f) -> vec4f { + return vec4f(sin(frag_coord.x), cos(frag_coord.y), 0.0, 1.0); + }").unwrap(); + // For bonus points: Process the base_module to rename all globals except for `main_image`. + let result = Frontend::new() + .parse_to_ast(" + @fragment + fn fs_main(@builtin(position) pos : vec4f, @location(0) uv: vec2f) -> vec4f { + return main_image(uv); + }") + .unwrap() + .to_module(Some(&base_module)) + .unwrap(); + ``` + ### Changes From 7c95ed76c3594a51073d24d7f19bf5ec20b99afe Mon Sep 17 00:00:00 2001 From: Stefnotch Date: Mon, 10 Jun 2024 09:37:25 +0200 Subject: [PATCH 10/11] Update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d5400547d..602799ed08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -104,7 +104,7 @@ By @stefnotch in [#5410](https://github.com/gfx-rs/wgpu/pull/5410) fn main_image(frag_coord: vec2f) -> vec4f { return vec4f(sin(frag_coord.x), cos(frag_coord.y), 0.0, 1.0); }").unwrap(); - // For bonus points: Process the base_module to rename all globals except for `main_image`. + // A full shadertoy implementation would rename all globals from base_module, except for `main_image`. let result = Frontend::new() .parse_to_ast(" @fragment From 50504e6e43600372457b79b27dfdda70dae43a8b Mon Sep 17 00:00:00 2001 From: Stefnotch Date: Mon, 10 Jun 2024 15:09:11 +0200 Subject: [PATCH 11/11] Remove unnecessary test --- naga/src/front/wgsl/tests.rs | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/naga/src/front/wgsl/tests.rs b/naga/src/front/wgsl/tests.rs index 2f8ba9d67d..31697d40fd 100644 --- a/naga/src/front/wgsl/tests.rs +++ b/naga/src/front/wgsl/tests.rs @@ -877,23 +877,3 @@ fn parse_base_module_storage_buffers() { .to_module(Some(&base_module)) .unwrap(); } - -/* Add this test once https://github.com/gfx-rs/wgpu/issues/5787 is fixed -#[test] -fn parse_base_module_storage_buffers_conflict() { - use crate::front::wgsl::Frontend; - - let base_module = parse_str( - "@group(0) @binding(0) - var foo: array;", - ) - .unwrap(); - let shader = "@group(0) @binding(0) - var bar: array;"; - let result = Frontend::new() - .parse_to_ast(shader) - .unwrap() - .to_module(Some(&base_module)); - assert!(result.is_err()); -} - */