[Mesa-dev] [PATCH 4/8] glsl: Add new builtins required by GL_ARB_sample_shading

[Ian Romanick]

On 10/16/2013 11:29 AM, Kenneth Graunke wrote:
> On 10/16/2013 10:40 AM, Ian Romanick wrote:
>> On 10/15/2013 07:50 PM, Kenneth Graunke wrote:
> [snip]
>>> Right, but gl_SampleMask is not an unsized array.  It's statically sized
>>> based on your implementation's maximum supported number of color samples
>>> (ceil(samples/32)), and this is totally known at compile time...
>>
>> That doesn't match what the spec says.  At least the GLSL 4.00 spec:
>>
>>     "In the fragment language, built-in variables are intrinsically
>>     declared as:
>>
>>         in vec4 gl_FragCoord;
>>         in bool gl_FrontFacing;
>>         in float gl_ClipDistance[];
>>         in vec2 gl_PointCoord;
>>         in int gl_PrimitiveID;
>>         in int gl_SampleID;
>>         in vec2 gl_SamplePosition;
>>         out vec4 gl_FragColor;
>>         out vec4 gl_FragData[gl_MaxDrawBuffers];
>>         out float gl_FragDepth;
>>         out int gl_SampleMask[];"
>>
>> Note that both gl_ClipDistance and gl_SampleMask are unsized, and both
>> have a maximum size based on implementation limits.  Later the GLSL 4.00
>> spec says:
>>
>>     "This array must be sized in the fragment shader either implicitly
>>     or explicitly to be the same size as the implementation-dependent
>>     maximum sample-mask (as an array of 32bit elements), determined by
>>     the maximum number of samples."
>>
>> The maximum size of the array is ceil(max_samples/32), but initially it
>> is unsized.
> 
> You've got to be kidding me.  This is utter and complete garbage.
> "We know exactly how big the array is, at compile time, but we're going
> to make you, the technical artist, tell us anyway.  By the way, you have
> to guess the exact number we're thinking."
> 
> So, about that "explicitly the same size" text...what if my shader sets
> it to some /other/ size?  What happens?
> 
> * Larger than ceil(float(gl_NumSamples)/32.0):
> 
> A compile error?  Truncated to the right size?  Probably the same
> behavior as ClipDistance.  That doesn't seem well specified either.
> 
> * Smaller than ceil(float(gl_NumSamples)/32.0):
> 
> From the GLSL 4.40 specification,
> "If the fragment shader statically assigns a value to gl_SampleMask,
>  the sample mask will be undefined for any array elements of any
>  fragment shader invocations that fail to assign a value."
> 
> The hardware is expecting a certain number of samples, so ultimately,
> the fact that you don't have them means they're undefined.  Just like
> the above case of forgetting to assign them.  Do we pretend the array is
> the right size, ignoring their request and treating any unwritten
> elements as unsized?  Or do we reject it as a compile error?
> 
> ...
> 
> Also, considering that the Radeon 8970 (arguably AMD's best card today)
> only does 24x MSAA, and it looks like nVidia's GTX Titan doesn't do more
> than 32x (though it's harder to tell), this array will always be exactly
> size 1 for every implementation in existence.

Looking that revision history, it was a fairly late change to make
gl_SampleMask an array at all.  It was just a scalar int.

> <sarcasm>
> I'm sure applications are fully on board to explicitly declare this and
> cope with the consequences of it not being a single integer.
> </sarcasm>

Maybe.  I doubt there are very many applications that use gl_SampleMask.
 Of the ones that do, they probably also use gl_SampleMaskIn and do
something like:

    gl_SampleMask = gl_SampleMaskIn; // of course this fails. duh

or

    for (...)
        gl_SampleMask[i] = gl_SampleMaskIn[i];

Note that the spec says "implicitly or explicitly".  The latter form
implicitly sizes gl_SampleMask to be the same size as gl_SampleMaskIn.
In ARB_gpu_shader5, gl_SampleMaskIn has somewhat more sensible language:

      in int gl_SampleMaskIn[];

    The variable gl_SampleMaskIn is an array of integers, each holding a
    bitfield indicating the set of samples covered by the primitive
    generating the fragment during multisample rasterization.  The
    array has ceil(<s>/32) elements, where <s> is the maximum number of
    color samples supported by the implementation.  Bit <n> or word <w>
    in the bitfield is set if and only if the sample numbered
    <w>*32+<n> is considered covered for this fragment shader
    invocation.

> That said, at this point, it's about 4 years too late to change the
> idiocy of the spec, so we're kind of stuck.  We probably ought to
> investigate what other vendors do.

You are completely correct here.  We should check what other vendors do.
 I think 5 tests will tell us everything we need to know (then we'll
probably submit some spec bugs).  I don't have any non-Intel hardware
that supports either extension, so I can't actually try any of these.

1. Does this compile, link, and run:

#version 130
#extension GL_ARB_sample_shading: require

out vec4 color;

void main() {
    color = vec4(gl_SampleMask.length());
    gl_SampleMask[0] = ~0;
}

2. Does this compile and link:

#version 130
#extension GL_ARB_sample_shading: require

out vec4 color;

void main() {
    color = vec4(1);
    gl_SampleMask[0] = ~0;
    gl_SampleMask[1] = ~0;
}

3. Does this compile and link:

#version 130
#extension GL_ARB_sample_shading: require
#extension GL_ARB_gpu_shader5: require

out vec4 color;

void main() {
    color = vec4(1);
    gl_SampleMask = gl_SampleMaskIn;
}

4. Does this compile and link:

#version 130
#extension GL_ARB_sample_shading: require

out vec4 color;
in int gl_SampleMask[1];

void main() {
    color = vec4(1);
    gl_SampleMask[0] = ~0;
}

5. Does this compile and link:

#version 130
#extension GL_ARB_sample_shading: require

out vec4 color;
in int gl_SampleMask[2];

void main() {
    color = vec4(1);
    gl_SampleMask[0] = ~0;
    gl_SampleMask[1] = ~0;
}

> --Ken