[Mesa-dev] [PATCH] glsl: Allow precision qualifiers for sampler types

[Anuj Phogat]

On Wed, Aug 28, 2013 at 4:56 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 08/27/2013 12:52 PM, Anuj Phogat wrote:
>> On Tue, Aug 27, 2013 at 11:53 AM, Ian Romanick <idr at freedesktop.org> wrote:
>>> On 08/27/2013 10:45 AM, Anuj Phogat wrote:
>>>>
>>>> GLSL 1.30 doesn't allow precision qualifiers on sampler types,
>>>> but in GLSL ES, sampler types are also allowed. This seems like
>>>> an oversight (since the intention of including these in GLSL 1.30
>>>> is to allow compatibility with ES shaders).
>>>>
>>>> Currently, Mesa allows "default" precision qualifiers to be set for
>>>> sampler types in GLSL (commit d5948f2). This patch makes it follow
>>>> GLSL ES rules and also allow declaring sampler variables with a
>>>> precision qualifier in GLSL.
>>>
>>>
>>> I think our current behavior is incorrect even in the ES case.  GLSL ES 3.30
>> You mean to say GLSL ES 3.00?
>
> Yes.  That's about the fifth time I've made that typo in the last week...
>
>>> and desktop GLSL 4.40 say the following in section 4.5.3 (Precision
>>> Qualifiers):
>>>
>>>
>>>     "Any floating point or any integer declaration can have the type
>>>     preceded by one of these precision qualifiers..."
>>>
>> Yes, samplers are now allowed in GLSL 4.4. They were not in GLSL 4.3.
>>
>>> The also both say the following in section 4.5.4 (Default Precision
>>> Qualifiers):
>>>
>>>     "The precision statement...can be used to establish a default
>>>     precision qualifier. The type field can be either int or float
>>>     or any of the sampler types..."
>>>
>>> So I believe
>>>
>>> precision mediump sampler2D;
>>>
>>> should be legal in all versions, but
>>>
>>> uniform mediump sampler2D s;
>>>
>>> should not.
>>>
>> Yes, there is no clear statement in GLSL spec which allows:
>> uniform mediump sampler2D s;
>>
>>> Which syntax is the test using?
>>>
>> test uses:
>> uniform mediump sampler2D s;
>>
>> I haven't yet tested if it is accepted by NVIDIA.
>
> There is an example in section 8 (Built-in Functions) that uses this syntax:
>
>     uniform lowp sampler2D sampler;
>     highp vec2 coord;
>     ...
>     lowp vec4 col = texture (sampler, coord); // texture() returns lowp
>
> It seems that this syntax should be legal.  I've submitted a spec bug to
> clarify the language in section 4.5.
>
> I have also attached a patch to fix up the comment in that piece of
> code.  Go ahead and combine my patch (with my Signed-off-by) with your
> code changes.
Thanks for finding this text from GLSL ES spec. I'll update the comments as
suggested. I think this patch also qualify for the mesa-stable mailing list.
>
> With the one other change suggested below,
>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>
>>>> This fixes a shader compilation error in Khronos OpenGL conformance
>>>> test "depth_texture_mipmap".
>>>>
>>>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com
>>>> ---
>>>>   src/glsl/ast_to_hir.cpp | 14 +++++++++-----
>>>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>>>> index 192130a..b3d6d8c 100644
>>>> --- a/src/glsl/ast_to_hir.cpp
>>>> +++ b/src/glsl/ast_to_hir.cpp
>>>> @@ -3131,8 +3131,8 @@ ast_declarator_list::hir(exec_list *instructions,
>>>>            state->check_precision_qualifiers_allowed(&loc);
>>>>         }
>>>>
>>>> -
>>>> -      /* Precision qualifiers only apply to floating point and integer
>>>> types.
>>>> +      /* Precision qualifiers apply to floating point, integer and
>>>> sampler
>>>> +       * types.
>>>>          *
>>>>          * From section 4.5.2 of the GLSL 1.30 spec:
>>>>          *    "Any floating point or any integer declaration can have the
>>>> type
>>>> @@ -3144,20 +3144,24 @@ ast_declarator_list::hir(exec_list *instructions,
>>>>          *
>>>>          * From page 87 of the GLSL ES spec:
>>>>          *    "RESOLUTION: Allow sampler types to take a precision
>>>> qualifier."
>>>> +       *
>>>> +       * GLSL 1.30 doesn't allow precision qualifiers on sampler types,
>>>> but
>>>> +       * this seems like an oversight (since the intention of including
>>>> these
>>>> +       * in GLSL 1.30 is to allow compatibility with ES shaders). So we
>>>> allow
>>>> +       * int, float, and all sampler types regardless of GLSL version.
>>>>          */
>>>>         if (this->type->qualifier.precision != ast_precision_none
>>>>             && !var->type->is_float()
>>>>             && !var->type->is_integer()
>>>>             && !var->type->is_record()
>>>> -          && !(var->type->is_sampler() && state->es_shader)
>>>> +          && !(var->type->is_sampler())
>
> You can delete the extra ( and ).
>
>>>>             && !(var->type->is_array()
>>>>                  && (var->type->fields.array->is_float()
>>>>                      || var->type->fields.array->is_integer()))) {
>>>>
>>>>            _mesa_glsl_error(&loc, state,
>>>>                             "precision qualifiers apply only to floating
>>>> point"
>>>> -                          "%s types", state->es_shader ? ", integer, and
>>>> sampler"
>>>> -                                                      : "and integer");
>>>> +                          ", integer and sampler types");
>>>>         }
>>>>
>>>>         /* From page 17 (page 23 of the PDF) of the GLSL 1.20 spec:
>