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

[Ian Romanick]

On 10/16/2013 10:40 AM, Ian Romanick wrote:
> On 10/15/2013 07:50 PM, Kenneth Graunke wrote:
>> On 10/15/2013 01:58 PM, Ian Romanick wrote:
>>> On 10/15/2013 01:50 PM, Anuj Phogat wrote:
>>>> On Tue, Oct 15, 2013 at 10:02 AM, Ian Romanick <idr at freedesktop.org> wrote:
>>>>> On 10/14/2013 10:12 AM, Anuj Phogat wrote:
>>>>>> @@ -789,6 +794,12 @@ builtin_variable_generator::generate_fs_special_vars()
>>>>>>        if (state->AMD_shader_stencil_export_warn)
>>>>>>           var->warn_extension = "GL_AMD_shader_stencil_export";
>>>>>>     }
>>>>>> +
>>>>>> +   if (state->ARB_sample_shading_enable) {
>>>>>> +      add_input(VARYING_SLOT_SAMPLE_ID, int_t, "gl_SampleID");
>>>>>> +      add_input(VARYING_SLOT_SAMPLE_POS, vec2_t, "gl_SamplePosition");
>>>>>> +      add_output(FRAG_RESULT_SAMPLE_MASK, array(int_t, 1), "gl_SampleMask");
>>>>>
>>>>> I don't see code anywhere in this patch series that correctly sizes this
>>>>> array.  I thought gl_SampleMask.length() == gl_NumSamples... except you
>>>>> can't use .length() on gl_SampleMask because the size can't be known at
>>>>> compile-time.  We should have a test that 'gl_SampleMask.length()' fails
>>>>> to compile. :)
>>>>>
>>>> I'll add a piglit compiler test for that.
>>>>
>>>>> The only other array that works like this is gl_TexCoord.  That array is
>>>>> sized to 0 (line 837 of builtin_variables.cpp).  I believe gl_SampleMask
>>>>> should do the same.
>>>>>
>>>> Yes, I noticed  gl_TexCoord is using array size of 0 and it works for
>>>> gl_SampleMask as well.  But, It generates same glsl ir with array size
>>>> of 0 or 1:
>>>> (declare (shader_out ) (array int 1) gl_SampleMask)
>>>>
>>>> what's the utility of using size 0?
>>>> I'll take care of rest of your comments in this patch.
>>>
>>> Using size 0 is like writing
>>>
>>> int gl_SampleMask[]; // unsized
>>>
>>> in the shader.  Later during compilation, usually in linking, unsized
>>> arrays become sized.  Somewhere between generating the built-in variable
>>> and printing the IR a size is established for it.
>>
>> 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."

But the 4.40 spec fixed the madness:

    "For both the input array gl_SampleMaskIn[] and the output array
    gl_SampleMask[], bit B of mask M (gl_SampleMaskIn[M] or
    gl_SampleMask[M]) corresponds to sample 32*M+B. These arrays
    have ceil(s/32) elements, where s is the maximum number of color
    samples supported by the implementation."

Setting the array size to 1 is correct after all.  Ugh.

> The maximum size of the array is ceil(max_samples/32), but initially it
> is unsized.
> 
>> --Ken
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev