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.

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

[hlsl-out] Arrays in Structs Are Incorrectly Loaded When Loaded as A Whole Struct #2184

Closed
Tracked by #2719
cwfitzgerald opened this issue Dec 27, 2022 · 3 comments · Fixed by #2281
Closed
Tracked by #2719
Assignees
Labels
area: back-end Outputs of shader conversion kind: bug Something isn't working lang: HLSL High-Level Shading Language

Comments

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Dec 27, 2022

WGSL

struct Ah {
    inner: array<f32, 2>,
};
@group(0) @binding(0)
var<storage> ah: Ah;

@compute @workgroup_size(1)
fn cs_main() {
    _ = ah;
}

HLSL

struct Ah {
    float inner[2];
};

ByteAddressBuffer ah : register(t0);

Ah ConstructAh(float arg0[2]) {
    Ah ret = (Ah)0;
    ret.inner = arg0;
    return ret;
}

[numthreads(1, 1, 1)]
void cs_main()
{
    // ERROR: Unexpected {
    Ah unnamed = ConstructAh({asfloat(ah.Load(0+0)), asfloat(ah.Load(0+4))});
}

Solution

As far as I can tell, the only real solution is to make the array part of the initialization a temporary. I wish there was a way to initialize an array inline, but I don't think you can. I'm not entirely sure how to do it but this code is the code that needs to be assigned to a temporary.

crate::TypeInner::Array {
base,
size: crate::ArraySize::Constant(const_handle),
..
} => {
write!(self.out, "{{")?;
let count = module.constants[const_handle].to_array_length().unwrap();
let stride = module.types[base].inner.size(&module.constants);
let iter = (0..count).map(|i| (TypeResolution::Handle(base), stride * i));
self.write_storage_load_sequence(module, var_handle, iter, func_ctx)?;
write!(self.out, "}}")?;
}
.

[numthreads(1, 1, 1)]
void cs_main()
{
    float temp[2] = {asfloat(ah.Load(0+0)), asfloat(ah.Load(0+4))};
    Ah unnamed = ConstructAh(temp);
}
@cwfitzgerald cwfitzgerald added kind: bug Something isn't working area: back-end Outputs of shader conversion lang: HLSL High-Level Shading Language labels Dec 27, 2022
@teoxoy teoxoy added this to the WGSL Specification V1 milestone Dec 27, 2022
@teoxoy
Copy link
Member

teoxoy commented Dec 27, 2022

We can inject a function to do inline init of arrays (we have this mechanism already).

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Feb 27, 2023

I'll take a look at this in the next few days weeks. EDIT: duration

@teoxoy
Copy link
Member

teoxoy commented Mar 13, 2023

The mechanism for this is already in place but we only use it for structs right now.

naga/src/back/hlsl/help.rs

Lines 876 to 881 in 8a72b7a

let constructor = WrappedConstructor { ty };
if !writer.wrapped.constructors.contains(&constructor) {
writer
.write_wrapped_constructor_function(module, constructor)?;
writer.wrapped.constructors.insert(constructor);
}

this is where we detect and inject the function that constructs the type

let constructor = super::help::WrappedConstructor {
ty: result_ty.handle().unwrap(),
};
self.write_wrapped_constructor_function_name(module, constructor)?;

this is where we inject a call to the function that constructs the type

We need to more or less copy-paste the code above for arrays.

ErichDonGubler added a commit to ErichDonGubler/naga that referenced this issue Mar 20, 2023
ErichDonGubler added a commit to ErichDonGubler/naga that referenced this issue Mar 20, 2023
ErichDonGubler added a commit to ErichDonGubler/naga that referenced this issue Mar 20, 2023
teoxoy pushed a commit that referenced this issue Mar 20, 2023
* test(hlsl-out): add failing case for array as ctor arg

See issue #2184.

* fix(hlsl): emit constructor functions for arrays
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: back-end Outputs of shader conversion kind: bug Something isn't working lang: HLSL High-Level Shading Language
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants