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

[Tomasz Figa]

Hi Ian,

An update on my testing inline.

On Tue, Sep 26, 2017 at 7:13 AM, Tomasz Figa <tfiga at chromium.org> wrote:
> 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.

Just ran the whole suite for GLES 3.0 conformance run with i965 driver
on Android and there is only 1 failure that seems to be unrelated to
the workaround (fails both with and without the change):

KHR-GLES3.shaders.uniform_block.common.precision_matching

However, I just realized that the applications affected are all using
GLSL ES 1.00. So, if we turn the condition introduced by my patch into
"(existing->data.used && var->data.used) || prog->data->Version >=
300", we should be safe both on compatibility and conformance side.

>>
>> 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.

I just tested it now and it doesn't fix the problem. However, I
misunderstood the code before. The shaders affected are not using
uniform blocks, so "!var->get_interface_type()" hits and leads to the
error.

Best regards,
Tomasz