[hlsl-out] fix matCx2 translation for uniform buffers #1802
Conversation
There was a problem hiding this comment.
This is very serious work, and it's done impressively well.
Thank you!
Small nits are noted.
Medium size question is present as well.
Finally, the biggest question I have to you is the following. Do you think it would be simpler to just give up on structured uniform buffers and represent them as arrays of vec4 internally? That's what Tint does, last time I checked. I quite like our approach because it's more debuggable, but the amount of code for the "exceptions" like matCx2 is a bit worrying me.
src/back/hlsl/help.rs
Outdated
| INDENT, RETURN_VARIABLE_NAME, field_name, ARGUMENT_VARIABLE_NAME, i, | ||
| )?; | ||
|
|
||
| match &module.types[member.ty].inner { |
There was a problem hiding this comment.
let's not match on a reference here, just match on a thing itself instead
src/back/hlsl/help.rs
Outdated
| )?; | ||
|
|
||
| match &module.types[member.ty].inner { | ||
| &crate::TypeInner::Matrix { columns, rows, .. } |
There was a problem hiding this comment.
nit: should just match as rows: crate::VectorSize::Bi
src/back/hlsl/help.rs
Outdated
| "{}.{}_{}", | ||
| STRUCT_ARGUMENT_VARIABLE_NAME, field_name, i | ||
| )?; | ||
| if i < columns as u8 - 1 { |
There was a problem hiding this comment.
small trick: if you move this condition to the beginning of the loop body (instead of the end), you can just compare i != 0
src/back/hlsl/help.rs
Outdated
|
|
||
| let field_name = &self.names[&NameKey::StructMember(access.ty, access.index)]; | ||
|
|
||
| match &module.types[member.ty].inner { |
There was a problem hiding this comment.
similarly, let's not match on a reference
src/back/hlsl/help.rs
Outdated
|
|
||
| let field_name = &self.names[&NameKey::StructMember(access.ty, access.index)]; | ||
|
|
||
| match &module.types[member.ty].inner { |
src/back/hlsl/help.rs
Outdated
| let member = &members[index as usize]; | ||
|
|
||
| match module.types[member.ty].inner { | ||
| crate::TypeInner::Matrix { rows, .. } |
There was a problem hiding this comment.
nit: let's encode the condition into the pattern here: rows: crate::VectorSize::Bi
src/back/hlsl/writer.rs
Outdated
| for i in 0..columns as u8 { | ||
| self.write_value_type(module, &vec_ty)?; | ||
| write!(self.out, " {}_{}", &self.names[&field_name_key], i)?; | ||
| if i < columns as u8 - 1 { |
There was a problem hiding this comment.
same little trick could be done here
src/back/hlsl/writer.rs
Outdated
| @@ -1215,11 +1237,175 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> { | |||
| writeln!(self.out, "[_i] = _result[_i];")?; | |||
| writeln!(self.out, "{}}}", level)?; | |||
| } else { | |||
| // We treat matrices of the form matCx2 as a sequence of C vec2's due to | |||
There was a problem hiding this comment.
please make sure to write something down at the top doc comment of mod.rs about this
|
|
||
| if !self.wrapped.struct_matrix_access.contains(&access) { | ||
| self.write_wrapped_struct_matrix_get_function(module, access)?; | ||
| self.write_wrapped_struct_matrix_set_function(module, access)?; |
There was a problem hiding this comment.
I suspect this is right but still would like to hear your thoughts on one question. Since the transformation this PR is doing is really only needed for matCx2 in uniform buffers, why do we care about set_xxx functions? Uniform buffers are read-only after all.
There was a problem hiding this comment.
I thought handling loads was all we needed as well in the beginning but realized that structs used for buffers can also be created in the shader and therefore we need to handle writes/stores as well. I included this scenario in the test file.
A different way to go about it would be to clone the struct declaration and only do this translation for the struct used in the constant buffer. The downside would be that we would need even more translation code to convert between the 2 versions in cases where functions take any of the structs as args.
I think with this PR we will be at a stage where we are covering all the discrepancies (between the buffer layout of WGSL and HLSL) since to get full compatibility with WGSL's uniform buffers we need to:
I've put together a resource covering all the differences between layouts that we can reference. What we might not be able to cover (but could with an array of vec4s) in the future is SPIR-V (with all its buffer layout flexibility) -> HLSL; I'm unsure if this in scope for naga or not (if it is maybe generating DXIL would be a better fit for this?). |
teoxoy
force-pushed
the
fix-hlsl-matCx2-translation
branch
from
df0e580 to
8b8be9d
Compare
Partially fixes gfx-rs/wgpu#4371 (GLSL backend still needs to be changed)