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] Add more padding when necessary #1814

Merged
merged 7 commits into from
Apr 12, 2022

Conversation

teoxoy
Copy link
Member

@teoxoy teoxoy commented Apr 9, 2022

fixes #1772

Add padding at the end of structs and after struct members of type matrix and array since apparently the size of HLSL's matrices, arrays and structs doesn't get rounded up to a multiple of their alignment implicitly.

see also #1772 (comment)

You can now see that the HLSL output of the newly added test has the expected offsets.
https://shader-playground.timjones.io/86acbc3f9bec2bad5d3668e5ac707eeb

@teoxoy
Copy link
Member Author

teoxoy commented Apr 9, 2022

Looks like the HLSL backend doesn't handle array constructors that well.

This WGSL

fn test(a: array<f32, 1>) -> f32 {
    return a[0];
}

@stage(vertex)
fn vertex() -> @builtin(position) vec4<f32> {
    return vec4<f32>(1.0) * test(array<f32, 1>(4.0));
}

produces this HLSL

float test(float a[1])
{
    return a[0];
}

float4 vertex() : SV_Position
{
    const float _e4 = test({ 4.0 });
    return (float4(1.0.xxxx) * _e4);
}

which DXC errors on with the same error in the CI

D:\local\Temp\340e7002-2bc1-4ebc-a460-75337a3d949f.hlsl:9:28: error: expected expression
    const float _e4 = test({ 4.0 });
                           ^

https://shader-playground.timjones.io/d3480954041d4f0515e1fada1d464256

@teoxoy
Copy link
Member Author

teoxoy commented Apr 9, 2022

Got it working by adding wrapped constructors for arrays.

Also added support for multidimensional arrays for the HLSL and GLSL backends and fixed arrays not working as function arguments.

src/back/glsl/mod.rs Outdated Show resolved Hide resolved
src/back/hlsl/conv.rs Outdated Show resolved Hide resolved
@kvark
Copy link
Member

kvark commented Apr 11, 2022

This looks quite reasonable, I don't have serious concerns.
Other than a bigger question of whether we should just give up and make it a float4 raw_data[] representation for uniform buffers in HLSL, which I think I asked in another PR. What do you think?

@teoxoy teoxoy force-pushed the hlsl-out-add-padding-for-aggregate-types branch from 1cfa7d3 to e1f211b Compare April 11, 2022 09:34
@teoxoy
Copy link
Member Author

teoxoy commented Apr 11, 2022

Other than a bigger question of whether we should just give up and make it a float4 raw_data[] representation for uniform buffers in HLSL, which I think I asked in another PR. What do you think?

Well, hopefully this should be the last edge case.

Tbh I like the addition of this "transparent" padding more. DXC shows offsets for types in buffers which makes it a lot easier to check if something is wrong. I would imagine it's easier to optimize accesses when DXC does the translation to DXIL too since it would just see the padding is not used and therefore remove it. I would also think that reconstructing the types from a vec4 array is a lot more involved than inserting padding in the right spots. Overall I don't see clear advantages to using a vec4 array.

What's your take on vec4 array vs padding?

@kvark
Copy link
Member

kvark commented Apr 12, 2022

What's your take on vec4 array vs padding?

I think having proper structures is more user (read: debugging) friendly, and I'd love for it to stay as long as it doesn't involve too much work for workarounds.
Thank you for implementing this!

@kvark kvark merged commit bd62887 into gfx-rs:master Apr 12, 2022
@teoxoy teoxoy deleted the hlsl-out-add-padding-for-aggregate-types branch April 12, 2022 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shader working on VULKAN but not on DX12 FRAGMENT
2 participants