KhronosGroup / SPIRV-Cross

SPIRV-Cross is a practical tool and library for performing reflection on SPIR-V and disassembling SPIR-V back to high level languages.
Apache License 2.0
2.03k stars 559 forks source link

MSL: Invalid code generated with matrix temporary #2302

Closed jmccarthy634 closed 5 months ago

jmccarthy634 commented 6 months ago

Hello, have a somewhat annoying to reproduce issue with float4x4 or float4x3 temporaries, it seems that sometimes the MSL generated from source HLSL that looks like this:

StructuredBuffer<float4x4> InXforms : register(t0);
...
float4x4 tmWorld = InXforms[viewData.gModelIndex];

Will end up generating MSL like this:

const device auto& InXforms = (const device type_StructuredBuffer_mat4v4float&)(*spvDescriptorSet0.viewData);

Which in turn gives errors like this:

Metal Error: program_source:46:35: error: C-style cast from 'const constant type_viewData' to 'const device type_StructuredBuffer_mat4v4float &' is not allowed
    const device auto& InXforms = (const device type_StructuredBuffer_mat4v4float&)(*spvDescriptorSet0.viewData);
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I've managed to create a simple repro of one we saw generated by a compute kernel for a vertex shader with the vertex_for_tessellation option enabled, but we've also seen this affect regular compute shaders as well. I've attached said repro with the source HLSL, SPIR-V and disassembly from DXC and the final output MSL from spirv-cross.

broken_mat4.zip

HansKristian-Work commented 5 months ago

This is because you're aliasing a UBO and SSBO over the same binding and using both. This is not valid SPIR-V.

HansKristian-Work commented 5 months ago

If the SSBO or UBO uses binding 1 instead, then it compiles fine:

vertex main0_out main0(main0_in in [[stage_in]], constant type_viewData& viewData [[buffer(0)]], const device type_StructuredBuffer_mat4v4float& InXforms [[buffer(1)]])
{
    main0_out out = {};
    out.gl_Position = viewData.viewData.gViewProjection * (InXforms._m0[viewData.viewData.gModelIndex] * float4(in.in_var_POSITION, 1.0));
    return out;
}
jmccarthy634 commented 5 months ago

Hi, it occurs to me that the SPIR-V is taken from DXC but before a step in which we modify the arguments to increment all buffer bindings from 0 to N, to avoid this overlap - I will endeavor to provide the actual SPIR-V that we feed into SPIRV-Cross with all post-compile modifications, so that this issue can be reopened.

EDIT: I guess in light of what you've said though there is probably a bug on our end with how we are doing that...