[Mesa-dev] [PATCH] glsl: Allow precision mismatch on dead data with GLSL ES 1.00

[Tomasz Figa]

On Wed, Sep 27, 2017 at 9:29 AM, Ian Romanick <idr at freedesktop.org> wrote:
> NAK.
>
> You mention varyings in the commit message, but then you quote spec text
> about uniforms.

Sorry, the part about varyings was a bad copy paste from the old
workaround patch on the bugzilla. The problem here is about uniforms.

> This bugged me, so I actually went and looked at the
> GLSL ES 1.00 specification.
>
> Section 4.3.5 (Varying) of the GLSL ES 1.00 specification says:
>
>     The precision of varying variables does not need to match.
>
> SO... for GLSL ES 1.00 we should not even emit a warning for varyings
> with mismatched precision.  The above spec quotation should appear in
> the patch that does this.  We should also add a piglit test and,
> frankly, a CTS test!  We've been rejecting completely valid shaders for
> over a year now, and the CTS should have caught that.
>
> If there are actually shaders in the wild that expect mismatched
> precision on uniforms in GLSL ES 1.00 shaders, we should deal with that
> in a separate patch.

Given the commit message corrected to refer to "uniforms" instead,
would it remove your NAK?

Best regards,
Tomasz

>
> On 09/26/2017 01:35 AM, Tomasz Figa wrote:
>> Commit 259fc505454ea6a67aeacf6cdebf1398d9947759 added linker error for
>> mismatching uniform precision, as required by GLES 3.0 specification and
>> conformance test-suite.
>>
>> Several Android applications, including Forge of Empires, have shaders
>> which violate this rule, on a dead varying that will be eliminated.
>> The problem affects a big number of applications using Cocos2D engine
>> and other GLES implementations accept this, this poses a serious
>> application compatibility issue.
>>
>> Starting from GLSL ES 3.0, declarations with conflicting precision
>> qualifiers are explicitly prohibited. However GLSL ES 1.00 does not
>> clearly specify the behavior, except that
>>
>>   "Uniforms are defined to behave as if they are using the same storage in
>>   the vertex and fragment processors and may be implemented this way.
>>   If uniforms are used in both the vertex and fragment shaders, developers
>>   should be warned if the precisions are different. Conversion of
>>   precision should never be implicit."
>>
>> The word "used" is not clear in this context and might refer to
>>  1) declared (same as GLES 3.x)
>>  2) referred after post-processing, or
>>  3) linked after all optimizations are done.
>>
>> Looking at existing applications, 2) or 3) seems to be widely adopted.
>> To avoid compatibility issues, turn the error into a warning if GLSL ES
>> version is lower than 3.0 and the data is dead in at least one of the
>> shaders.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97532
>> Signed-off-by: Tomasz Figa <tfiga at chromium.org>
>> ---
>>  src/compiler/glsl/linker.cpp | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
>> index f352c5385ca..ff2e6b7a0d3 100644
>> --- a/src/compiler/glsl/linker.cpp
>> +++ b/src/compiler/glsl/linker.cpp
>> @@ -1121,10 +1121,16 @@ cross_validate_globals(struct gl_shader_program *prog,
>>           if (prog->IsES && (prog->data->Version != 310 ||
>>                              !var->get_interface_type()) &&
>>               existing->data.precision != var->data.precision) {
>> -            linker_error(prog, "declarations for %s `%s` have "
>> -                         "mismatching precision qualifiers\n",
>> -                         mode_string(var), var->name);
>> -            return;
>> +            if ((existing->data.used && var->data.used) || prog->data->Version >= 300) {
>> +               linker_error(prog, "declarations for %s `%s` have "
>> +                            "mismatching precision qualifiers\n",
>> +                            mode_string(var), var->name);
>> +               return;
>> +            } else {
>> +               linker_warning(prog, "declarations for %s `%s` have "
>> +                              "mismatching precision qualifiers\n",
>> +                              mode_string(var), var->name);
>> +            }
>>           }
>>        } else
>>           variables->add_variable(var);
>>
>