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

[Ian Romanick]

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.

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:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: comment-fix-up.patch
Type: application/pgp-keys
Size: 1933 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130828/cdf587eb/attachment-0001.key>