[Mesa-dev] [PATCH] glsl: Trigger precision mismatch error only if both symbols are used

[Tomasz Figa]

On Tue, Sep 26, 2017 at 6:21 AM, Tomasz Figa <tfiga at chromium.org> wrote:
> Hi Ian,
>
> On Tue, Sep 26, 2017 at 5:59 AM, Ian Romanick <idr at freedesktop.org> wrote:
>> On 09/25/2017 02:53 AM, Tomasz Figa wrote:
>>> Commit 259fc505454ea6a67aeacf6cdebf1398d9947759 added linker error for
>>> mismatching uniform precision, as required by GLES 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, so work around the problem
>>> by erroring out only if both symbols are actually referenced in the
>>> code, which is the only case when the mismatch can cause incorrect
>>> behavior.
>>
>> I think this change will cause failures in the CTS... but, there's a
>> Khronos bug about this issue, and I think some of the rules are going to
>> get relaxed.  I'd like to wait until that gets sorted before we start
>> changing how our linker applies the rules.
>
> Are we 100% sure about the failures? There are multiple GLES
> implementations that claim to be compliant, yet they allow such
> behavior.

I'm looking through Khronos CTS github repository and it seems like a
related test (dEQP-GLES3.functional.shaders.linkage.uniform.block.differing_precision)
was removed from GLES 3 suite and instead few others added to GLES 3.1
suite:

https://github.com/KhronosGroup/VK-GL-CTS/commit/b8d0d0a842280c2594703ecce985563aebe1cde8#diff-72bd3f02df978e071f2a53ec2654ddd5

Moreover I can see a test being included in GLES 3.2 suite
(KHR-GLES32.shaders.negative.unused_uniform_precision_matching) that
prohibits precision mismatch even for unused uniforms, but it is not
included in any suites for earlier GLES versions.

Best regards,
Tomasz

>
> Note that this is affecting a huge number of Android applications,
> including a commonly used 2D game engine and we're having a really big
> problem right now, because even if the engine gets fixed, it's very
> unlikely that all the developers update their engine.
>
>>
>>> Based on Kenneth Graunke's patch from Bugzilla, reworked from a drirc
>>> option that completely bypasses the check into an incoditional check
>>
>>                                                     unconditional
>>
>>> that triggers either an error or warning, respectively if both
>>> declarations are further referenced by the code or not.
>>>
>>> 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..1c534ea1a3b 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 ||
>>>
>>
>> Does just removing the "prog->data->Version != 310" clause also fix the
>> problem?
>
> Will check later today, but I suspect it would, since the applications
> are not GLES 3.1.
>
> Best regards,
> Tomasz