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.

Sign up for GitHub

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

Jump to bottom

[WIP] update naga to 0.13.0 #37

Closed
wants to merge 4 commits into from
Closed

[WIP] update naga to 0.13.0 #37

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0152fd0
update naga to 0.13.0
VitalyAnkh Jul 24, 2023
eeb62e3
elide useless expr
VitalyAnkh Jul 25, 2023
e07caee
Merge branch 'master' into update-naga-to-0.13
VitalyAnkh Jul 25, 2023
46e7ded
Use RefCell<T> other than &mut T
VitalyAnkh Jul 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ prune = []
override_any = []

[dependencies]
naga = { version="0.12", features = ["wgsl-in", "wgsl-out", "glsl-in", "glsl-out", "clone", "span"] }
naga = { version="0.13", features = ["wgsl-in", "wgsl-out", "glsl-in", "glsl-out", "clone", "span"] }
tracing = "0.1"
regex = "1.5"
regex-syntax = "0.6"
Expand All @@ -29,6 +29,6 @@ once_cell = "1.17.0"
indexmap = "1.9.3"

[dev-dependencies]
wgpu = { version = "0.16", features=["naga"] }
wgpu = { version = "0.17", features=["naga"] }
futures-lite = "1"
tracing-subscriber = { version = "0.3", features = ["std", "fmt"] }
101 changes: 84 additions & 17 deletions src/derive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use indexmap::IndexMap;
use naga::{
Arena, ArraySize, Block, Constant, ConstantInner, EntryPoint, Expression, Function,
FunctionArgument, FunctionResult, GlobalVariable, Handle, ImageQuery, LocalVariable, Module,
Arena, ArraySize, Block, Constant, EntryPoint, Expression, Function, FunctionArgument,
FunctionResult, GlobalVariable, Handle, ImageQuery, LocalVariable, Module, Override,
SampleLevel, Span, Statement, StructMember, SwitchCase, Type, TypeInner, UniqueArena,
};
use std::collections::HashMap;
Expand All @@ -13,11 +13,13 @@ pub struct DerivedModule<'a> {

type_map: HashMap<Handle<Type>, Handle<Type>>,
const_map: HashMap<Handle<Constant>, Handle<Constant>>,
const_expression_map: HashMap<Handle<Expression>, Handle<Expression>>,
global_map: HashMap<Handle<GlobalVariable>, Handle<GlobalVariable>>,
function_map: HashMap<String, Handle<Function>>,

types: UniqueArena<Type>,
constants: Arena<Constant>,
const_expressions: Arena<Expression>,
globals: Arena<GlobalVariable>,
functions: Arena<Function>,
}
Expand Down Expand Up @@ -104,7 +106,7 @@ impl<'a> DerivedModule<'a> {
}
TypeInner::Array { base, size, stride } => {
let size = match size {
ArraySize::Constant(c) => ArraySize::Constant(self.import_const(c)),
c @ ArraySize::Constant(_) => *c,
robtfm marked this conversation as resolved.
Show resolved Hide resolved
ArraySize::Dynamic => ArraySize::Dynamic,
};
TypeInner::Array {
Expand All @@ -115,7 +117,7 @@ impl<'a> DerivedModule<'a> {
}
TypeInner::BindingArray { base, size } => {
let size = match size {
ArraySize::Constant(c) => ArraySize::Constant(self.import_const(c)),
c @ ArraySize::Constant(_) => *c,
ArraySize::Dynamic => ArraySize::Dynamic,
};
TypeInner::BindingArray {
Expand Down Expand Up @@ -145,16 +147,14 @@ impl<'a> DerivedModule<'a> {

let new_const = Constant {
name: c.name.clone(),
specialization: c.specialization,
inner: match &c.inner {
ConstantInner::Scalar { .. } => c.inner.clone(),
ConstantInner::Composite { ty, components } => {
let components = components.iter().map(|c| self.import_const(c)).collect();
ConstantInner::Composite {
ty: self.import_type(ty),
components,
}
}
r#override: c.r#override.clone(),
ty: self.import_type(&c.ty),
//TODO: infinite recursion?
init: if c.r#override == Override::None {
self.import_const_expression(&c.init)
} else {
// r#override doesn't do anything now
Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks to me like init and override can both be specified - init is the default if an override is not provided on the pipeline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the override doesn't do anything at this time. Maybe we should wait for naga to add functionality for this override field?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i'd just import/copy both the fields over. no reason not to

unreachable!()
},
};

Expand Down Expand Up @@ -183,7 +183,7 @@ impl<'a> DerivedModule<'a> {
space: gv.space,
binding: gv.binding.clone(),
ty: self.import_type(&gv.ty),
init: gv.init.map(|c| self.import_const(&c)),
init: gv.init.map(|c| self.import_const_expression(&c)),
};

let span = self
Expand All @@ -199,6 +199,63 @@ impl<'a> DerivedModule<'a> {
new_h
})
}
// remap a const expression from source context into our derived context
pub fn import_const_expression(
&mut self,
const_expression_handle: &Handle<Expression>,
) -> Handle<Expression> {
self.const_expression_map
.get(const_expression_handle)
.copied()
.unwrap_or_else(|| {
let old_c = self
.shader
.as_ref()
.unwrap()
.const_expressions
.try_get(*const_expression_handle)
.unwrap();

let new_const = match old_c {
Expression::Constant(h_c) => {
let c = self
.shader
.as_ref()
.unwrap()
.constants
.try_get(*h_c)
.unwrap();

Constant {
name: c.name.clone(),
r#override: c.r#override.clone(),
ty: self.import_type(&c.ty),
init: self.import_const_expression(&c.init),
}
}
// TODO
Copy link
Collaborator

Choose a reason for hiding this comment

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

const_expressions can be more than just constants, they can be compound expressions as long as all the components are const_expressions as well.

i haven't looked in a lot of detail, but i think rather than making a new function here, the easier thing to do is to use the existing import_expression fn passing self.const_expressions and self.shader.const_expressions in as the new and old arenas (instead of the function-local arenas that are used when import_expression is called in function import).

you will probably need to also make a member for the already_imported param, that gets reset when the shader is changed. this might mean we end up with duplicate const_expressions if multiple modules define the same constant, but that should be ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use the existing import_expression fn passing self.const_expressions and self.shader.const_expressions in as the new and old arenas

With this approach I'm struggling with the borrow checker 🤕 🤕 🤕 The import_expression both borrows self.const_expressions and self.shader.const_expressions exclusively. Please give me some time to fix this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think it would be pretty reasonable to change the already_imported and new_expressions params to RefCells (as well as the const_expressions and const_expressions_map members).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed them to RefCells but there are many runtime BorrowMut errors when running tests, still working at it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think you should change everything to refcell, just the const_expression bits (and the params for import_expression). i should have time at the weekend for a proper look if you get stuck.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I didn't change everything to RefCell, I can't change the &mut self to &self, which occurs in import_block, import_const and other import functions, then borrow checkings errors happens: self.import_expression(...&self.const_expression_map, &self.const_expressions,...) 🤕

i should have time at the weekend for a proper look if you get stuck.

Appreciate it! I'm going on a trip tomorrow so maybe I don't have much time at the weekend, but I will try my best to work with you and make this update happen.

// What should I do?
_ => todo!(),
};

let span = self
.shader
.as_ref()
.unwrap()
.const_expressions
.get_span(*const_expression_handle);
let new_constant_handle = self
.constants
.fetch_or_append(new_const, self.map_span(span));
let new_const_expression = naga::Expression::Constant(new_constant_handle);
let new_const_expression_handle = self
.const_expressions
.append(new_const_expression, self.map_span(span));
self.const_expression_map
.insert(*const_expression_handle, new_const_expression_handle);
new_const_expression_handle
})
}

// remap a block
fn import_block(
Expand Down Expand Up @@ -325,6 +382,12 @@ impl<'a> DerivedModule<'a> {
value: map_expr!(value),
result: map_expr!(result),
},
Statement::WorkGroupUniformLoad { pointer, result } => {
Statement::WorkGroupUniformLoad {
pointer: map_expr!(pointer),
result: map_expr!(result),
}
}
Statement::Return { value } => Statement::Return {
value: map_expr_opt!(value),
},
Expand Down Expand Up @@ -406,6 +469,8 @@ impl<'a> DerivedModule<'a> {
let mut is_external = false;
let expr = old_expressions.try_get(h_expr).unwrap();
let expr = match expr {
Expression::Literal(_) => expr.clone(),
Expression::ZeroValue(zv) => Expression::ZeroValue(self.import_type(zv)),
Expression::CallResult(f) => Expression::CallResult(self.map_function_handle(f)),
Expression::Constant(c) => {
is_external = true;
Expand Down Expand Up @@ -434,7 +499,7 @@ impl<'a> DerivedModule<'a> {
gather: *gather,
coordinate: map_expr!(coordinate),
array_index: map_expr_opt!(array_index),
offset: offset.map(|c| self.import_const(&c)),
offset: offset.map(|c| self.import_const_expression(&c)),
level: match level {
SampleLevel::Auto | SampleLevel::Zero => *level,
SampleLevel::Exact(expr) => SampleLevel::Exact(map_expr!(expr)),
Expand Down Expand Up @@ -549,6 +614,7 @@ impl<'a> DerivedModule<'a> {
}

Expression::AtomicResult { .. } => expr.clone(),
Expression::WorkGroupUniformLoadResult { .. } => expr.clone(),
Expression::RayQueryProceedResult => expr.clone(),
Expression::RayQueryGetIntersection { query, committed } => {
Expression::RayQueryGetIntersection {
Expand Down Expand Up @@ -591,7 +657,7 @@ impl<'a> DerivedModule<'a> {
let new_local = LocalVariable {
name: l.name.clone(),
ty: self.import_type(&l.ty),
init: l.init.map(|c| self.import_const(&c)),
init: l.init.map(|c| self.import_const_expression(&c)),
};
let span = func.local_variables.get_span(h_l);
let new_h = local_variables.append(new_local, self.map_span(span));
Expand Down Expand Up @@ -688,6 +754,7 @@ impl<'a> From<DerivedModule<'a>> for naga::Module {
types: derived.types,
constants: derived.constants,
global_variables: derived.globals,
const_expressions: derived.const_expressions,
functions: derived.functions,
special_types: Default::default(),
entry_points: Default::default(),
Expand Down
1 change: 1 addition & 0 deletions src/redirect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ impl Redirector {
| Statement::Break
| Statement::Continue
| Statement::Return { .. }
| Statement::WorkGroupUniformLoad { .. }
| Statement::Kill
| Statement::Barrier(_)
| Statement::Store { .. }
Expand Down