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

[Anuj Phogat]

On Wed, Oct 16, 2013 at 3:49 PM, Ian Romanick <idr at freedesktop.org> wrote:
> 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.
>
I'll try these tests on AMD and NVIDIA hadware.

> 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
>