MSL: Unable to dynamically access an incoming gl_ClipDistance array under tessellation

[billhollings]

SPIR-V conversion to MSL fails with error:

MSL conversion error: Trying to dynamically index into an array interface variable in tessellation. This is currently unsupported.

The attempt in GLSL results in gl_in[0].gl_ClipDistance[_80], but in MSL main0_in, the incoming gl_ClipDistance array is flattened in to float gl_ClipDistance_0 [[attribute(8)]];.

Is this the result of a known limitation in MSL, or is it something we can enhance SPIRV-Cross to emit and access the incoming gl_ClipDistance as an array? @cdavis5e

These CTS test fail as a result:

dEQP-VK.tessellation.user_defined_io.per_vertex_block.vertex_io_array_size_implicit.triangles
dEQP-VK.tessellation.user_defined_io.per_vertex_block.vertex_io_array_size_implicit.quads
dEQP-VK.tessellation.user_defined_io.per_vertex_block.vertex_io_array_size_shader_builtin.triangles
dEQP-VK.tessellation.user_defined_io.per_vertex_block.vertex_io_array_size_shader_builtin.quads
dEQP-VK.tessellation.user_defined_io.per_vertex_block.vertex_io_array_size_spec_min.triangles
dEQP-VK.tessellation.user_defined_io.per_vertex_block.vertex_io_array_size_spec_min.quads
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.1
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.1_fragmentshader_read
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.2
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.2_fragmentshader_read
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.3
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.3_fragmentshader_read
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.4
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.4_fragmentshader_read
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.5
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.6_fragmentshader_read
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.6
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.6_fragmentshader_read
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.7
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.7_fragmentshader_read
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.8
dEQP-VK.clipping.user_defined.clip_distance_dynamic_index.vert_tess.8_fragmentshader_read
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess.8
dEQP-VK.clipping.user_defined.clip_cull_distance_dynamic_index.vert_tess.8_fragmentshader_read

SPIR-V file:
tess_dyn_idx_clip_dist.spv.zip

[cdavis5e]

Is this the result of a known limitation in MSL

Yes and no. In the fragment shader case, it is, because MSL doesn't allow arrays to be used for shader stage I/O, other than for the express purpose of outputting [[clip_distance]]s from a vertex shader. In the tessellation shader case, it's more because remapping the access chains in this case got very hairy.

is it something we can enhance SPIRV-Cross to emit and access the incoming gl_ClipDistance as an array?

Theoretically, yes. For fragment shaders, we could declare an array to hold the incoming clip distances and index that. For tessellation shaders, we could just declare the clip distances as an array, since it's just a regular buffer. Remapping the access chain in this case could be problematic.

[HansKristian-Work]

This is an absolute nightmare to handle. One way to handle this perhaps would be to think outside the box. If the cull/clip count can be limited to 4, we could read it as a float4 attribute, which allows us to index it properly.

EDIT: Nah, minspec is 8 :\

[billhollings]

Nah, minspec is 8 :\

However...if this was the most appropriate solution, then the portability extension could define and return the lower value of 4 for maxClipDistances.

[aitor-lunarg]

Just dumping some thoughts, unsure how feasible it could be.

What about having the array decomposed into a structure with members and then building a local array in the next stage with said structure. I believe Metal should be okay with this?

Example:
Vertex GLSL

// Whatever inputs for position

layout(location = 0) out vec2 outSomeData[10];

void main()
{
    // Set position and fill outSomeData
}

Metal equivalent

// Input structure

// Our defined structure to handle the array
struct outSomeDataStructure {
    float2 value0;
    float2 value1;
    // ...
    float2 value9;
};
struct MyVertexOut {
    // position
    outSomeDataStructure outSomeData
};
vertex MyVertexOut myVertex(MyVertexIn in [[stage_in]])
{
    vec2 tempSomeDataStructure[10] = { 0 };
    // Fill tempSomeDataStructure as in GLSL

    // Copy tempSomeDataStructure to output struct
    result.outSomeData.value0 = tempSomeDataStructure[0];
    result.outSomeData.value1 = tempSomeDataStructure[1];
    // ...
    result.outSomeData.value9 = tempSomeDataStructure[9];
}

Same thing applies for the fragment, but the other way around, we need to first fill the local array. There's no real need to have a structure just for the array in GLSL and directly add the values to the output structure in Metal.

While the approach may not be the best, it should be a good enough workaround at the cost of some performance.

As of now, more tests than the ones mentioned in the original report of the issue are affected by this and make them fail (Vulkan 1.0 example):

dEQP-VK.pipeline.monolithic.multisample_interpolation.nonuniform_interpolant_indexing.centroid
dEQP-VK.pipeline.monolithic.multisample_interpolation.nonuniform_interpolant_indexing.sample
dEQP-VK.pipeline.monolithic.multisample_interpolation.nonuniform_interpolant_indexing.offset

Let me know your thoughts!

[ncesario-lunarg]

However...if this was the most appropriate solution, then the portability extension could define and return the lower value of 4 for maxClipDistances.

I don't think that helps the tests that Aito mentioned?

https://github.com/ncesario-lunarg/SPIRV-Cross/tree/issue-1796 is a very rough hack that attempts to address the general "indexing into an array with pull interpolants" problem. This is done by creating a new array for each interpolation operation seen, for each input array. This is obviously not ideal from a size and perf standpoint, but I'm not sure how one could fix this in general otherwise. The change referenced appears to get all of the tests posted here passing with the exception of tests that involve interpolateAtOffset.

@HansKristian-Work if you get a chance to take a look, could you please advise on whether this approach is worth pursuing, or if you would rather go a different route here?

[billhollings]

FWIW...

The 24 CTS failures in the OP above are no longer occurring.

The 3 additional CTS failures that @aitor-lunarg added:

dEQP-VK.pipeline.monolithic.multisample_interpolation.nonuniform_interpolant_indexing.centroid
dEQP-VK.pipeline.monolithic.multisample_interpolation.nonuniform_interpolant_indexing.sample
dEQP-VK.pipeline.monolithic.multisample_interpolation.nonuniform_interpolant_indexing.offset

are still occurring, but report a different error:

SPIR-V to MSL conversion error: Trying to dynamically index into an array interface variable using pull-model interpolation. This is currently unsupported.

[ncesario-lunarg]

Thanks for taking a look @billhollings. I must have had an old version of CTS and/or MoltenVK on my system as I'm pretty sure those were not passing when I first looked at this 🤷‍♂️ .

This change is for the general case of passing arrays between shader stages. It fixes the "sample" and "centroid" cases Aitor referenced, but not the "offset" case. For the offset case (and in general), some constant propagation would do the trick, unless we could perhaps use something like spirv-opt in MoltenVK before passing the spirv to spirv-cross?