[Mesa-dev] [PATCH] glsl/linker: fix bug when checking precision qualifier

[Tapani Pälli]

On 27.02.2018 15:07, Samuel Iglesias Gonsálvez wrote:
> 
> 
> On 27/02/18 13:37, Tapani Pälli wrote:
>>
>>
>> On 01/30/2018 09:50 AM, Samuel Iglesias Gonsálvez wrote:
>>> According to GLSL ES 3.2 spec, see table in 9.2.1 "Linked Shaders"
>>> section, the precision qualifier should match for uniform variables.
>>> This also applies to previous GLSL ES 3.x specs.
>>>
>>> This 'if' checks the condition for uniform variables, while for UBOs
>>> it is checked in link_interface_blocks.cpp.
>>>
>>> Fixes: b50b82b8a553
>>> ("glsl/es31: precision qualifier doesn't need to match in shader
>>> interface block members")
>>>
>>> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>>> ---
>>>    src/compiler/glsl/linker.cpp | 12 ++++--------
>>>    1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
>>> index ce101935b01..050b2906f6b 100644
>>> --- a/src/compiler/glsl/linker.cpp
>>> +++ b/src/compiler/glsl/linker.cpp
>>> @@ -1134,15 +1134,11 @@ cross_validate_globals(struct
>>> gl_shader_program *prog,
>>>                  return;
>>>             }
>>> -         /* Only in GLSL ES 3.10, the precision qualifier should not
>>> match
>>> -          * between block members defined in matched block names
>>> within a
>>> -          * shader interface.
>>> -          *
>>> -          * In GLSL ES 3.00 and ES 3.20, precision qualifier for
>>> each block
>>> -          * member should match.
>>> +
>>> +         /* Check the precision qualifier matches for uniform
>>> variables. For UBOs,
>>> +          * it is checked in link_interface_blocks.cpp.
>>
>> I think this change is correct. However this comment started to bother
>> me, there was commit fc6d55952d08ea03d133c1178871b0d4d289a0cf that
>> says "glsl/es: precision qualifier doesn't need to match in UBOs". Do
>> we have precision qualifier matching for UBOs?
>>
> 
> On GLSL ES they don't need to match, only on desktop GLSL. My intention
> here was to indicate where such check is done for UBO inside Mesa for
> desktop GLSL, not implying it is needed for GLSL ES at all. Sorry for
> the confusion.
> 
> I can change the comment as:
> 
> "Check the precision qualifier matches for uniform variables on GLSL ES"
> 
> ... and forget about the rest, if you agree.

Yeah, I think this would be nice and clear!

I also verified the mentioned 'Linked Shaders' table from GLSL ES 3.2 
and 3.1 specs. I assume CI is happy as well;

Reviewed-by: Tapani Pälli <tapani.palli at intel.com>


> Sam
> 
>>
>>>              */
>>> -         if (prog->IsES && (prog->data->Version != 310 ||
>>> -                            !var->get_interface_type()) &&
>>> +         if (prog->IsES && !var->get_interface_type() &&
>>>                 existing->data.precision != var->data.precision) {
>>>                if ((existing->data.used && var->data.used) ||
>>> prog->data->Version >= 300) {
>>>                   linker_error(prog, "declarations for %s `%s` have "
>>>
>>
> 

// Tapani