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.02k stars 558 forks source link

MSL: why is a std140 mat3 compiled to packed_float3x3? #1088

Closed bejado closed 5 years ago

bejado commented 5 years ago

Given the following GLSL:

#version 410

layout(std140) uniform ObjectUniforms
{
    mat3 worldFromModelNormalMatrix;
} objectUniforms;

layout(location = 6) out vec3 vertex_worldTangent;

void main()
{
    vertex_worldTangent = objectUniforms.worldFromModelNormalMatrix * vec3(1.0, 0.0, 0.0);
}

SPIRV-cross on the latest master now emits a packed_float3x3 for worldFromModelNormalMatrix along with a typedef:

typedef packed_float3 packed_float3x3[3];
struct ObjectUniforms
{
    packed_float3x3 worldFromModelNormalMatrix;
};

Isn't the correct type here a MSLfloat3x3? Previously SPIRV-cross output float3x3, which has the same packing as GLSL's mat3 under std140. This commit seems suspect.

Command-line:

$ glslangValidator -G -o test.spv test.frag
$ spirv-cross --msl test.spv
cdavis5e commented 5 years ago

Sorry about that. I was working on support for scalar block layout, and I think I broke this for you. It seems to be marking that member as packed, when it shouldn't be, since float3 has the correct 16-byte alignment and size for std140 layout. I'll look into this--we're aware that code is in need of serious refactoring (see #1074).

bejado commented 5 years ago

No problem! Thanks for the quick response

HansKristian-Work commented 5 years ago

Yup, fallout from the scalar matrix support, surprised this wasn't caught in testing, sorry about that. Stepped through and the check goes here: https://github.com/KhronosGroup/SPIRV-Cross/blob/master/spirv_msl.cpp#L2571. Previously there would have been an early out path, and I suspect that was changed quite a lot for scalar support. @cdavis5e I'll wait for a PR to address this issue.

I think I'll start thinking about the packing refactor seriously in the meantime ...