[Mesa-dev] glsl: Add a citation for uniform precision matching.

Submitted by Kenneth Graunke on Sept. 6, 2016, 7:18 p.m.

Details

Message ID 20160906191846.3629-1-kenneth@whitecape.org
State New
Headers show
Series "glsl: Add a citation for uniform precision matching." ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Kenneth Graunke Sept. 6, 2016, 7:18 p.m.
Ian added this check in commit 259fc505454ea6a67aeacf6cdebf1398d9947759.
While reviewing the rules, I found a citation which spells this out
clearly, so I figured I'd send a patch to add it as a comment.

Cc: idr@freedesktop.org
Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
---
 src/compiler/glsl/linker.cpp | 8 ++++++++
 1 file changed, 8 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
index c95edf3..78c9ea8 100644
--- a/src/compiler/glsl/linker.cpp
+++ b/src/compiler/glsl/linker.cpp
@@ -1154,6 +1154,14 @@  cross_validate_globals(struct gl_shader_program *prog,
             return;
          }
 
+         /* The GLSL ES 3.2 specification says:
+          *
+          *    "Uniforms in shaders all share a single global name space when
+          *     linked into a program or separable program. Hence, the types,
+          *     precisions and any location specifiers of all declared uniform
+          *     variables with the same name must match across shaders that
+          *     are linked into a single program."
+          */
          if (prog->IsES && existing->data.precision != var->data.precision) {
             linker_error(prog, "declarations for %s `%s` have "
                          "mismatching precision qualifiers\n",

Comments

Kenneth Graunke <kenneth@whitecape.org> writes:

> Ian added this check in commit 259fc505454ea6a67aeacf6cdebf1398d9947759.
> While reviewing the rules, I found a citation which spells this out
> clearly, so I figured I'd send a patch to add it as a comment.
>
> Cc: idr@freedesktop.org
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>

Reviewed-by: Eric Anholt <eric@anholt.net>

I was wondering about this change, because glmark2 is failing to compile
its terrain shaders now.
On Tuesday, September 6, 2016 1:04:43 PM PDT Eric Anholt wrote:
> Kenneth Graunke <kenneth@whitecape.org> writes:
> 
> > Ian added this check in commit 259fc505454ea6a67aeacf6cdebf1398d9947759.
> > While reviewing the rules, I found a citation which spells this out
> > clearly, so I figured I'd send a patch to add it as a comment.
> >
> > Cc: idr@freedesktop.org
> > Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>
> 
> I was wondering about this change, because glmark2 is failing to compile
> its terrain shaders now.

Really?  GLBenchmark 2.7 also fails to compile:

https://bugs.freedesktop.org/show_bug.cgi?id=97532

I'm beginning to doubt whether any other vendor implements this part of
the spec, or if they have some variation of it.
On 09/06/2016 03:24 PM, Kenneth Graunke wrote:
> On Tuesday, September 6, 2016 1:04:43 PM PDT Eric Anholt wrote:
>> Kenneth Graunke <kenneth@whitecape.org> writes:
>>
>>> Ian added this check in commit 259fc505454ea6a67aeacf6cdebf1398d9947759.
>>> While reviewing the rules, I found a citation which spells this out
>>> clearly, so I figured I'd send a patch to add it as a comment.
>>>
>>> Cc: idr@freedesktop.org
>>> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
>>
>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>
>> I was wondering about this change, because glmark2 is failing to compile
>> its terrain shaders now.

Out of curiosity... is the uniform used in all the stages?

> Really?  GLBenchmark 2.7 also fails to compile:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=97532
> 
> I'm beginning to doubt whether any other vendor implements this part of
> the spec, or if they have some variation of it.

I added this specifically for a dEQP test.  Presumably people are
passing that.

dEQP-GLES31.functional.shaders.linkage.geometry.uniform.rules.type_mismatch_1.

I wonder if this is only enforced in GLSL ES 3.2 shaders?  I'll do some
spec archaeology to see if the language changed at all over the years.  Ugh.

> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On 09/07/2016 09:56 AM, Ian Romanick wrote:
> On 09/06/2016 03:24 PM, Kenneth Graunke wrote:
>> On Tuesday, September 6, 2016 1:04:43 PM PDT Eric Anholt wrote:
>>> Kenneth Graunke <kenneth@whitecape.org> writes:
>>>
>>>> Ian added this check in commit 259fc505454ea6a67aeacf6cdebf1398d9947759.
>>>> While reviewing the rules, I found a citation which spells this out
>>>> clearly, so I figured I'd send a patch to add it as a comment.
>>>>
>>>> Cc: idr@freedesktop.org
>>>> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
>>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>>
>>> I was wondering about this change, because glmark2 is failing to compile
>>> its terrain shaders now.
> Out of curiosity... is the uniform used in all the stages?
>
>> Really?  GLBenchmark 2.7 also fails to compile:
>>
>> https://bugs.freedesktop.org/show_bug.cgi?id=97532
>>
>> I'm beginning to doubt whether any other vendor implements this part of
>> the spec, or if they have some variation of it.
> I added this specifically for a dEQP test.  Presumably people are
> passing that.
>
> dEQP-GLES31.functional.shaders.linkage.geometry.uniform.rules.type_mismatch_1.
>
> I wonder if this is only enforced in GLSL ES 3.2 shaders?  I'll do some
> spec archaeology to see if the language changed at all over the years.  Ugh.

As mentioned in the bug 97532 GLSL ES 1.x spec seems a bit vague in this 
matter and talks about 'warning' (section 10 Issues).


>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Wednesday, September 7, 2016 10:42:06 AM PDT Tapani Pälli wrote:
> On 09/07/2016 09:56 AM, Ian Romanick wrote:
> > On 09/06/2016 03:24 PM, Kenneth Graunke wrote:
> >> On Tuesday, September 6, 2016 1:04:43 PM PDT Eric Anholt wrote:
> >>> Kenneth Graunke <kenneth@whitecape.org> writes:
> >>>
> >>>> Ian added this check in commit 259fc505454ea6a67aeacf6cdebf1398d9947759.
> >>>> While reviewing the rules, I found a citation which spells this out
> >>>> clearly, so I figured I'd send a patch to add it as a comment.
> >>>>
> >>>> Cc: idr@freedesktop.org
> >>>> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> >>> Reviewed-by: Eric Anholt <eric@anholt.net>
> >>>
> >>> I was wondering about this change, because glmark2 is failing to compile
> >>> its terrain shaders now.
> > Out of curiosity... is the uniform used in all the stages?
> >
> >> Really?  GLBenchmark 2.7 also fails to compile:
> >>
> >> https://bugs.freedesktop.org/show_bug.cgi?id=97532
> >>
> >> I'm beginning to doubt whether any other vendor implements this part of
> >> the spec, or if they have some variation of it.
> > I added this specifically for a dEQP test.  Presumably people are
> > passing that.
> >
> > dEQP-GLES31.functional.shaders.linkage.geometry.uniform.rules.type_mismatch_1.
> >
> > I wonder if this is only enforced in GLSL ES 3.2 shaders?  I'll do some
> > spec archaeology to see if the language changed at all over the years.  Ugh.
> 
> As mentioned in the bug 97532 GLSL ES 1.x spec seems a bit vague in this 
> matter and talks about 'warning' (section 10 Issues).

It seems pretty clear to me...

GLSL ES 1.0.17, Section 10 Issues:

   "Do precision qualifiers for uniforms need to match?

    Option 1: Yes.
    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.

    Option 2: No.
    Uniforms may be used by both shaders but the same precision may not
    be available in both so there is a justification for allowing them
    to be different.

    Using the same uniform in the vertex and fragment shaders will
    always require the precision to be specified in the vertex shader
    (since the default precision is highp). This is an unnecessary
    burden on developers.

    RESOLUTION: Yes, precision qualifiers for uniforms must match."

The "options" are just suggestions that were discussed when drafting the
spec.  I believe the "developers should be warned" comment in option 1
is expressing the sentiment that silently handling mismatched precision
behind the developers' back is doing them a disservice.  Plus, the only
way to issue a warning but handle it would be to implicitly convert one
to a different precision...and they say it "should never be implicit."

Regardless, the important part is the resolution: they must match.
Even better, the actual body of the spec says they must match:

GLSL ES 1.0.17, Section 4.3.4 Uniform:

   "When the vertex and fragment shaders are linked together, then they
    will share a single global uniform name space. Hence, types and
    precisions of uniforms with the same name must match across all
    shaders that are linked into a single executable."

I noticed that there's several years of discussion in the Khronos
bugzilla, though, which may offer some insight (or a lot of confusion).

--Ken
On 09/07/2016 12:37 PM, Kenneth Graunke wrote:
> On Wednesday, September 7, 2016 10:42:06 AM PDT Tapani Pälli wrote:
>> On 09/07/2016 09:56 AM, Ian Romanick wrote:
>>> On 09/06/2016 03:24 PM, Kenneth Graunke wrote:
>>>> On Tuesday, September 6, 2016 1:04:43 PM PDT Eric Anholt wrote:
>>>>> Kenneth Graunke <kenneth@whitecape.org> writes:
>>>>>
>>>>>> Ian added this check in commit 259fc505454ea6a67aeacf6cdebf1398d9947759.
>>>>>> While reviewing the rules, I found a citation which spells this out
>>>>>> clearly, so I figured I'd send a patch to add it as a comment.
>>>>>>
>>>>>> Cc: idr@freedesktop.org
>>>>>> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
>>>>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>>>>
>>>>> I was wondering about this change, because glmark2 is failing to compile
>>>>> its terrain shaders now.
>>> Out of curiosity... is the uniform used in all the stages?
>>>
>>>> Really?  GLBenchmark 2.7 also fails to compile:
>>>>
>>>> https://bugs.freedesktop.org/show_bug.cgi?id=97532
>>>>
>>>> I'm beginning to doubt whether any other vendor implements this part of
>>>> the spec, or if they have some variation of it.
>>> I added this specifically for a dEQP test.  Presumably people are
>>> passing that.
>>>
>>> dEQP-GLES31.functional.shaders.linkage.geometry.uniform.rules.type_mismatch_1.
>>>
>>> I wonder if this is only enforced in GLSL ES 3.2 shaders?  I'll do some
>>> spec archaeology to see if the language changed at all over the years.  Ugh.
>> As mentioned in the bug 97532 GLSL ES 1.x spec seems a bit vague in this
>> matter and talks about 'warning' (section 10 Issues).
> It seems pretty clear to me...
>
> GLSL ES 1.0.17, Section 10 Issues:
>
>     "Do precision qualifiers for uniforms need to match?
>
>      Option 1: Yes.
>      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.
>
>      Option 2: No.
>      Uniforms may be used by both shaders but the same precision may not
>      be available in both so there is a justification for allowing them
>      to be different.
>
>      Using the same uniform in the vertex and fragment shaders will
>      always require the precision to be specified in the vertex shader
>      (since the default precision is highp). This is an unnecessary
>      burden on developers.
>
>      RESOLUTION: Yes, precision qualifiers for uniforms must match."
>
> The "options" are just suggestions that were discussed when drafting the
> spec.  I believe the "developers should be warned" comment in option 1
> is expressing the sentiment that silently handling mismatched precision
> behind the developers' back is doing them a disservice.  Plus, the only
> way to issue a warning but handle it would be to implicitly convert one
> to a different precision...and they say it "should never be implicit."
>
> Regardless, the important part is the resolution: they must match.
> Even better, the actual body of the spec says they must match:
>
> GLSL ES 1.0.17, Section 4.3.4 Uniform:
>
>     "When the vertex and fragment shaders are linked together, then they
>      will share a single global uniform name space. Hence, types and
>      precisions of uniforms with the same name must match across all
>      shaders that are linked into a single executable."

Yes agreed, this is true and is definitively stronger argument than 
'Issues section'.

> I noticed that there's several years of discussion in the Khronos
> bugzilla, though, which may offer some insight (or a lot of confusion).

:)

> --Ken


// Tapani
Ian Romanick <idr@freedesktop.org> writes:

> On 09/06/2016 03:24 PM, Kenneth Graunke wrote:
>> On Tuesday, September 6, 2016 1:04:43 PM PDT Eric Anholt wrote:
>>> Kenneth Graunke <kenneth@whitecape.org> writes:
>>>
>>>> Ian added this check in commit 259fc505454ea6a67aeacf6cdebf1398d9947759.
>>>> While reviewing the rules, I found a citation which spells this out
>>>> clearly, so I figured I'd send a patch to add it as a comment.
>>>>
>>>> Cc: idr@freedesktop.org
>>>> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
>>>
>>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>>
>>> I was wondering about this change, because glmark2 is failing to compile
>>> its terrain shaders now.
>
> Out of curiosity... is the uniform used in all the stages?

In glmark2 -b terrain, it's defined but not used in VS.