[msl-out] wrap arrays in structs so that they can be returned by functions

[expenses]

This fixes #741. Unfortunately it currently breaks the quad-vert.msl snapshot with this error:

Validating quad-vert.msl
<stdin>:40:30: error: type 'type6' is not valid for attribute 'clip_distance'
    type6 gl_ClipDistance1 [[clip_distance]];
                             ^~~~~~~~~~~~~
<stdin>:42:8: error: invalid return type 'main2Output' for vertex function
vertex main2Output main2(
       ^
<stdin>:54:86: error: type 'type6' does not provide a subscript operator
    const auto _tmp = type10 {v_uv, _.gl_Position, _.gl_PointSize, {_.gl_ClipDistance[0]}};
                                                                    ~~~~~~~~~~~~~~~~~^~
3 errors generated.

This right solution to fix this test would be for gl_ClipDistance to be treated as a float instead of a float[1] in msl-out.

[expenses]

This right solution to fix this test would be for gl_ClipDistance to be treated as a float instead of a float[1] in msl-out.

I guess we need some kind of type override system for this?

[kvark]

Thank you for taking a stab at this!

This right solution to fix this test would be for gl_ClipDistance to be treated as a float instead of a float[1] in msl-out.

Are you sure this is the right solution? I think there may be multiple clip distances, technically, so if we implement this workaround, we'll be back at square one with regards to general support for this.
Instead, the subject of this PR sounds more reasonable. I do wonder if we should simply wrap all of the arrays...

[expenses]

Are you sure this is the right solution? I think there may be multiple clip distances, technically, so if we implement this workaround, we'll be back at square one with regards to general support for this.
Instead, the subject of this PR sounds more reasonable. I do wonder if we should simply wrap all of the arrays...

Yeah I just realised that table 5.3 of https://developer.apple.com/metal/Metal-Shading-Language-Specification.pdf states that it can be an array of floats. I'll have a go at using the array type in the output struct so instead of type6 gl_ClipDistance1 [[clip_distance]]; we have float[1] gl_ClipDistance1 [[clip_distance]];

[expenses]

Instead, the subject of this PR sounds more reasonable. I do wonder if we should simply wrap all of the arrays...

To clarify, the reason we can't do is is because a wrapped array doesn't count for [[clip_distance]]:

<stdin>:40:30: error: type 'type6' is not valid for attribute 'clip_distance'
    type6 gl_ClipDistance1 [[clip_distance]];
                             ^~~~~~~~~~~~~

[expenses]

I think what I've got here works pretty well now. Just FYI, what spirv-cross does in these situations is that it creates a template struct like:

template<typename T, size_t Num>
struct spvUnsafeArray
{
    T elements[Num ? Num : 1];
    
    thread T& operator [] (size_t pos) thread
    {
        return elements[pos];
    }
    constexpr const thread T& operator [] (size_t pos) const thread
    {
        return elements[pos];
    }
    
    device T& operator [] (size_t pos) device
    {
        return elements[pos];
    }
    constexpr const device T& operator [] (size_t pos) const device
    {
        return elements[pos];
    }
    
    constexpr const constant T& operator [] (size_t pos) const constant
    {
        return elements[pos];
    }
    
    threadgroup T& operator [] (size_t pos) threadgroup
    {
        return elements[pos];
    }
    constexpr const threadgroup T& operator [] (size_t pos) const threadgroup
    {
        return elements[pos];
    }
};

and returns that in functions instead:

static inline __attribute__((always_inline))
spvUnsafeArray<float, 1> xyz()
{
    spvUnsafeArray<float, 1> y;
    return y;
}

This means that they don't have to do .inner at all.

[kvark]

Amazing stuff, I'm excited to see this happening!
I'd prefer to make this work with inner instead of defining this complex template in the code like SPIRV-Cross does.

[kvark]

Are we all set?

[kvark]

wait, so here we can't just do _tmp.gl_ClipDistance1 for the last argument?

[expenses]

Nope, unfortunately not. Not gl_ClipDistance1.inner either.

[kvark]

hmm, I'm concerned now, a little bit. So when we are initializing the I/O struct, we can't just pass _tmp.gl_ClipDistance1 because it's an array. But how would this work in other places? I.e. if you have a user function returning a user struct, then passing { xxx.inner[0], xxx.inner[1] } would just fail horribly, since the only field of the target struct is inner.

[expenses]

Afaik, only having a single pair of those { brackets when constructing a struct is okay if it only has one field. I think more testing would be nice, but I haven't found a (trivial) example that this breaks. Something like the following, for example, is fine:

#version 450

vec3[8] hello_world() {
    vec3[8] xyz;
    return xyz;
}

struct X {
  vec3[8] yyy;
};

vec3[8] hmm2() {
  X x;
  x.yyy = hello_world();
  x.yyy[1] = vec3(3.0);
  return x.yyy;
}

layout(location = 0) out vec4 colour;

void main() {
    vec3[8] xyz = hmm2();
    colour = vec4(xyz[7], 1.0);
}

->

#include <metal_stdlib>
#include <simd/simd.h>

struct type3 {
    metal::float3 inner[8u];
};
struct X {
    type3 yyy;
};
constant metal::float3 const_type1_ = {3.0, 3.0, 3.0};

type3 hello_world(
) {
    type3 xyz;
    return xyz;
}

type3 hmm2_(
) {
    X x;
    type3 _e13 = hello_world();
    for(int _i=0; _i<8; ++_i) x.yyy.inner[_i] = _e13.inner[_i];
    x.yyy.inner[1] = const_type1_;
    return x.yyy;
}

void main1(
    thread metal::float4& colour
) {
    type3 xyz1;
    type3 _e13 = hmm2_();
    for(int _i=0; _i<8; ++_i) xyz1.inner[_i] = _e13.inner[_i];
    metal::float3 _e15 = xyz1.inner[7];
    colour = metal::float4(_e15.x, _e15.y, _e15.z, 1.0);
    return;
}

struct main2Output {
    metal::float4 member [[color(0)]];
};
fragment main2Output main2(
) {
    metal::float4 colour = {};
    main1(colour);
    return main2Output { colour };
}

[kvark]

I mean, specifically for arrays.

struct Foo {
  xxx: array<f32; 3>;
};
fn bar() -> Foo {
  return Foo(array<f32;3>(1.0, 2.0, 3.0));
}

My understanding is that this PR right now is not going to handle this. Ideally, we'd want it to generate something like

const auto _tmp = Foo { .. };
return { { _tmp.xxx[0], _tmp.xxx[1], _tmp.xxx[2] } };

This will fail because the produced Foo would be declared with the inner in it, but the return code doesn't use it.

[kvark]

Ok, I don't understand how type1 {1.0, 2.0, 3.0} part works :(
type1 is a structure with 1 element. So shouldn't it yell at us for providing 3 elements instead of 1?

[expenses]

Yeah I'm not 100% sure either, but what seems to happen is that (as I wrote in #764 (comment)):

Afaik, only having a single pair of those { brackets when constructing a struct is okay if it only has one field

[expenses]

Same thing is allowed in C:

struct Arr {
    float inner[4];
};

int main() {
    struct Arr arr = { 1.0, 1.5, 2.0, 2.5 };
}

[expenses]

Uh oh, in both MSL and C it lets you skip fields in the constructor though so

Foo bar1(
) {
    return Foo {type1 {1.0}, 2};
}

also works?? With no warnings or errors 😟 ??

[kvark]

What's going to happen if the array only has one element? How is the compiler going to figure out if the initializer is for this field or for the inner itself? And initializers don't even have to be provided for all the fields. Ugh, C is weird :(

[expenses]

Are we all set?

I think so! I hope this doesn't break any vertex shaders.

[kvark]

I'm royally confused about how C arrays work, and that is going to make the maintenance of this code to be difficult.
But you've really done a great job implementing it, and it addresses a very important issue we have, and there isn't any cleaner solution that we know. So let's proceed!