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
1.96k stars 549 forks source link

MSL: dynamic/runtime array conversion #2109

Open CryptoCrocodile opened 1 year ago

CryptoCrocodile commented 1 year ago

We're trying to convert a runtime array from GLSL -> SPIRV to MSL via SPIRV cross. The conversion works on Windows, if we look at the GLSL decompiled SPIRV via RenderDoc. On Mac, we get a statically sized array:

Here's the GLSL:

layout(set = 0, binding = 3, std430) readonly buffer boneMatrices
{
    vec4 _boneMatrices[];
} _216;

Here's the converted MSL:

struct boneMatrices
{
    float4 _boneMatrices[1];
};

The SPIRV looks correct as well:

%6 = OpTypeFloat 32
%27 = OpTypeVector %6 4
%213 = OpTypeRuntimeArray %27
%214 = OpTypeStruct %213
%215 = OpTypePointer StorageBuffer %214
%216 = OpVariable %215 StorageBuffer

We're using MoltenVK to do the translation, any tips would be appreciated in how we can facilitate the proper conversion.

Thanks.

HansKristian-Work commented 1 year ago

This is intentional. This is the classic "hack" to do runtime arrays in older C. MSL does not support C99 unsized arrays.

CryptoCrocodile commented 1 year ago

Thanks for the fast reply @HansKristian-Work! Are there some compatibility issues or caveats with this? We're seeing random issues here on metal, for instance the bone matrices returning invalid data for anything but the first index.

HansKristian-Work commented 1 year ago

This is used everywhere and I've never heard this being an issue before, so I don't think so.

CryptoCrocodile commented 1 year ago

Thanks, that helps!

teiasherman commented 1 year ago

MSL does not support C99 unsized arrays.

@HansKristian-Work the Apple docs use a pointer syntax for this (see here). Would it be difficult to output float4 * _boneMatrices; (for this example) instead of [1]?

I haven't seen any issues running the Metal shader. However the use of a constant sized array of 1 has caused some recurring confusing on my team. Developers end up pausing their work to confirm what they wrote in GLSL is correct. Then they investigate our cross-compiling framework and discover it was intentional.

We didn't look into changing our fork of spirv-cross yet. I thought I would ask here first after seeing this issue.

HansKristian-Work commented 1 year ago

Would it be difficult to output float4 * _boneMatrices; (for this example) instead of [1]?

It does not work in anything except for the most trivial cases as I've explained already (probably in some other issue). For SSBOs with composite types, this cannot work.

I haven't seen any issues running the Metal shader. However the use of a constant sized array of 1 has caused some recurring confusing on my team. Developers end up pausing their work to confirm what they wrote in GLSL is correct. Then they investigate our cross-compiling framework and discover it was intentional.

Maybe there could just be an inline comment in textual form to avoid that issue: [1 /* unsized array workaround */] or something.

teiasherman commented 1 year ago

It does not work in anything except for the most trivial cases as I've explained already (probably in some other issue). For SSBOs with composite types, this cannot work.

Ok, thank you. I'm not sure I follow completely. I admit I didn't try a composite type to see what would happen. I believe Metal expects a pointer for the array use case. Do you mean there is a limitation in the cross compiler logic, like it would only work for OpTypeRuntimeArray so it would still be confusing for other cases?

HansKristian-Work commented 1 year ago

Something like a typical C99 struct:

buffer SSBO
{
    float a;
    int b[10];
    vec4 c[4];
    uvec2 unsized_array[];
};

This is supported in SPIR-V and GLSL, but there is no way to use a plain pointer here.