[Metal] Metal backend doesn't know about gl_PointSize

[floooh]

It seems that the Metal backend currently doesn't add the [[point_size]] attribute to vertex shader outputs for the special GL variable gl_PointSize.

As example: the GLSL input to glsangValidator looks like this:

#version 330
uniform params {
    mat4 mvp;
    float psize;
};
in vec4 position;
in vec4 color0;
out vec4 color;
void main() {
    gl_Position = mvp * position;
    gl_PointSize = psize;
    color = color0;
}

And here's the resulting Metal shader (note the missing [[point_size]] on the gl_PointSize output:

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

using namespace metal;

struct params
{
    float4x4 mvp;
    float psize;
};

struct main0_in
{
    float4 color0 [[attribute(10)]];
    float4 position [[attribute(0)]];
};

struct main0_out
{
    float4 color [[user(locn0)]];
    float4 gl_Position [[position]];
    float gl_PointSize;
    float gl_ClipDistance[1] /* [[clip_distance]] built-in not yet supported under Metal. */;
};

vertex main0_out main0(main0_in in [[stage_in]], constant params& _19 [[buffer(0)]], uint gl_VertexID [[vertex_id]], uint gl_InstanceID [[instance_id]])
{
    main0_out out = {};
    out.gl_Position = _19.mvp * in.position;
    out.gl_PointSize = _19.psize;
    out.color = in.color0;
    return out;
}

[h3xl3r]

There's an is_rendering_points option in CompilerMSL::Options that makes the MSL backend add the attribute.

[HansKristian-Work]

If you know that gl_PointSize is used by the shader, is there a reason why [[point_size]] cannot be used automatically? Ping @billhollings.

[billhollings]

I'm starting to look into this now.

[billhollings]

The reason that is_rendering_points exists is because, when converting GLSL to SPIR-V, glslang often invents a gl_PerVertex interface struct that contains an unused gl_PointSize member.

CompilerMSL will dutifully put that gl_PointSize member in the output struct of a vertex function, even though it is not populated by the shader.

For example, see the GLSL input and MSL output for the flatten/basic.flatten.vert shader (and several others).

The problem is that during the compilation of the MSL shader into a render pipeline, Metal will throw an exception (on OSX, not always on iOS) if the shader defines a [[point_size]] output and the primitive topology is NOT MTLPrimitiveTopologyClassPoint, even though the variable is not populated by the shader.

As a result..there needs to be some way of turning off the [[point_size]] attribute when the shader is not really rendering points, and that is the purpose of the is_rendering_points parameter. For example, MoltenVK sets the is_rendering_points parameter accordingly during runtime shader compilation into render pipelines.

There are a couple of ways we could modify behaviour to resolve the issue being discussed...

1. The simplest is probably to leave the is_rendering_points parameter in place, but give it a default value of true instead of false. MoltenVK will turn that flag off if the topology is not points, and the assumption for other conversions will be that the shader includes gl_PointSize because it really did intend to output point size.

2. A more accurate implementation might be to determine whether the gl_PerVertex::gl_PointSize member is actually populated by the shader. If it is not, then we can elide it. We have the ability to determine whether a full variable is unused via the is_hidden_variable() function, but there seems to be no mechanism for detecting whether a struct member is used.

@HansKristian-ARM Can you comment on the viability of (2) please? What would it take to determine whether the gl_PerVertex::gl_PointSize struct member is really used by the shader?

[billhollings]

I've added PR #177 to implement (1) above. We can add (2) later if it looks feasible based on further discussion and review.

[HansKristian-Work]

There is a mechanism to determine now if a builtin variable is used statically by the shader now.
See active_input_builtins and active_output_builtins. HLSL uses it to determine which builtins should have proper semantic input/output from the shader.

It is still possible for a shader to write to gl_PointSize, but point rendering is not used, so some option to forcefully disable PointSize as a builtin should be possible (I don't think writing to gl_PointSize with non-point topology is an error in GL/Vulkan), but by default, I think static use of PointSize can be made into a proper semantic automatically.

[HansKristian-Work]

@billhollings The extra check here was fairly trivial so made a PR, please review when you have time: #178.

[floooh]

FYI, as per merge commit e876cdf this is now working for me. Each Metal vertex shader currently has a float gl_PointSize [[point_size]] output, but this doesn't seem to do any harm if it is unused (at least I'm not seeing any problems so far).

Thanks and looking forward to the remaining PRs :)

[HansKristian-Work]

Merged on master.