[Mesa-dev,v2,05/23] glsl: Fail to link if inter-stage input/outputs are not assigned to stream 0

Submitted by Iago Toral Quiroga on June 18, 2014, 9:51 a.m.

Details

Message ID 1403085110-31168-6-git-send-email-itoral@igalia.com
State Accepted
Commit 02fd80e16018d44374063bae703352c464fdec2c
Headers show

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga June 18, 2014, 9:51 a.m.
Outputs that are linked to inputs in the next stage must be output to stream 0,
otherwise we should fail to link.
---
 src/glsl/link_varyings.cpp | 8 ++++++++
 1 file changed, 8 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
index 9725a43..3b20594 100644
--- a/src/glsl/link_varyings.cpp
+++ b/src/glsl/link_varyings.cpp
@@ -1345,6 +1345,14 @@  assign_varying_locations(struct gl_context *ctx,
          if (input_var || (prog->SeparateShader && consumer == NULL)) {
             matches.record(output_var, input_var);
          }
+
+         /* Only stream 0 outputs can be consumed in the next stage */
+         if (input_var && output_var->data.stream != 0) {
+            linker_error(prog, "output %s is assigned to stream=%d but "
+                         "is linked to an input, which requires stream=0",
+                         output_var->name, output_var->data.stream);
+            return false;
+         }
       }
    } else {
       /* If there's no producer stage, then this must be a separable program.

Comments

Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

On 06/18/2014 02:51 AM, Iago Toral Quiroga wrote:
> Outputs that are linked to inputs in the next stage must be output to stream 0,
> otherwise we should fail to link.
> ---
>  src/glsl/link_varyings.cpp | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> index 9725a43..3b20594 100644
> --- a/src/glsl/link_varyings.cpp
> +++ b/src/glsl/link_varyings.cpp
> @@ -1345,6 +1345,14 @@ assign_varying_locations(struct gl_context *ctx,
>           if (input_var || (prog->SeparateShader && consumer == NULL)) {
>              matches.record(output_var, input_var);
>           }
> +
> +         /* Only stream 0 outputs can be consumed in the next stage */
> +         if (input_var && output_var->data.stream != 0) {
> +            linker_error(prog, "output %s is assigned to stream=%d but "
> +                         "is linked to an input, which requires stream=0",
> +                         output_var->name, output_var->data.stream);
> +            return false;
> +         }
>        }
>     } else {
>        /* If there's no producer stage, then this must be a separable program.
>
Hi,

Where does the spec say we should fail to link? I don't see such a
statement there.

It looks like varyings with stream > 0 should not be linked with the
fragment shader.

Marek

On Wed, Jun 18, 2014 at 11:51 AM, Iago Toral Quiroga <itoral@igalia.com> wrote:
> Outputs that are linked to inputs in the next stage must be output to stream 0,
> otherwise we should fail to link.
> ---
>  src/glsl/link_varyings.cpp | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> index 9725a43..3b20594 100644
> --- a/src/glsl/link_varyings.cpp
> +++ b/src/glsl/link_varyings.cpp
> @@ -1345,6 +1345,14 @@ assign_varying_locations(struct gl_context *ctx,
>           if (input_var || (prog->SeparateShader && consumer == NULL)) {
>              matches.record(output_var, input_var);
>           }
> +
> +         /* Only stream 0 outputs can be consumed in the next stage */
> +         if (input_var && output_var->data.stream != 0) {
> +            linker_error(prog, "output %s is assigned to stream=%d but "
> +                         "is linked to an input, which requires stream=0",
> +                         output_var->name, output_var->data.stream);
> +            return false;
> +         }
>        }
>     } else {
>        /* If there's no producer stage, then this must be a separable program.
> --
> 1.9.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Wed, 2015-07-29 at 21:58 +0200, Marek Olšák wrote:
> Hi,
> 
> Where does the spec say we should fail to link? I don't see such a
> statement there.

I have reviewed ARB_gpu_shader5 and I don't see any specific mentions to
what should be done in this particular case. That said, isn't this the
logical thing to do? It is a programming error to link an FS input to a
GS output bound to a non-zero stream and at best the program would have
undefined behavior if the FS input is used. Hiding this from the
developer silently does not seem to be a good idea in any case, whatever
the developer was trying to accomplish he is doing it wrong.

> It looks like varyings with stream > 0 should not be linked with the
> fragment shader.

How is this better?

FWIW, the proprietary nVidia driver also fails to link in this case with
this error:

"output 'var_name' is associated with an input with a non-zero stream,
which is not allowed"

Iago

> Marek
> 
> On Wed, Jun 18, 2014 at 11:51 AM, Iago Toral Quiroga <itoral@igalia.com> wrote:
> > Outputs that are linked to inputs in the next stage must be output to stream 0,
> > otherwise we should fail to link.
> > ---
> >  src/glsl/link_varyings.cpp | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> > index 9725a43..3b20594 100644
> > --- a/src/glsl/link_varyings.cpp
> > +++ b/src/glsl/link_varyings.cpp
> > @@ -1345,6 +1345,14 @@ assign_varying_locations(struct gl_context *ctx,
> >           if (input_var || (prog->SeparateShader && consumer == NULL)) {
> >              matches.record(output_var, input_var);
> >           }
> > +
> > +         /* Only stream 0 outputs can be consumed in the next stage */
> > +         if (input_var && output_var->data.stream != 0) {
> > +            linker_error(prog, "output %s is assigned to stream=%d but "
> > +                         "is linked to an input, which requires stream=0",
> > +                         output_var->name, output_var->data.stream);
> > +            return false;
> > +         }
> >        }
> >     } else {
> >        /* If there's no producer stage, then this must be a separable program.
> > --
> > 1.9.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On Thu, Jul 30, 2015 at 8:49 AM, Iago Toral <itoral@igalia.com> wrote:
> On Wed, 2015-07-29 at 21:58 +0200, Marek Olšák wrote:
>> Hi,
>>
>> Where does the spec say we should fail to link? I don't see such a
>> statement there.
>
> I have reviewed ARB_gpu_shader5 and I don't see any specific mentions to
> what should be done in this particular case. That said, isn't this the
> logical thing to do? It is a programming error to link an FS input to a
> GS output bound to a non-zero stream and at best the program would have
> undefined behavior if the FS input is used. Hiding this from the
> developer silently does not seem to be a good idea in any case, whatever
> the developer was trying to accomplish he is doing it wrong.
>
>> It looks like varyings with stream > 0 should not be linked with the
>> fragment shader.
>
> How is this better?

The spec simply says that varyings with stream > 0 are not passed to
the rasterizer, which means the matching fragment shader inputs should
be uninitialized, but it should still be possible to capture the
varyings with transform feedback. That's how I understand the spec.

Marek
On Thu, 2015-07-30 at 09:43 +0200, Marek Olšák wrote:
> On Thu, Jul 30, 2015 at 8:49 AM, Iago Toral <itoral@igalia.com> wrote:
> > On Wed, 2015-07-29 at 21:58 +0200, Marek Olšák wrote:
> >> Hi,
> >>
> >> Where does the spec say we should fail to link? I don't see such a
> >> statement there.
> >
> > I have reviewed ARB_gpu_shader5 and I don't see any specific mentions to
> > what should be done in this particular case. That said, isn't this the
> > logical thing to do? It is a programming error to link an FS input to a
> > GS output bound to a non-zero stream and at best the program would have
> > undefined behavior if the FS input is used. Hiding this from the
> > developer silently does not seem to be a good idea in any case, whatever
> > the developer was trying to accomplish he is doing it wrong.
> >
> >> It looks like varyings with stream > 0 should not be linked with the
> >> fragment shader.
> >
> > How is this better?
> 
> The spec simply says that varyings with stream > 0 are not passed to
> the rasterizer, which means the matching fragment shader inputs should
> be uninitialized, but it should still be possible to capture the
> varyings with transform feedback. That's how I understand the spec.

FWIW, I have tested this in the proprietary nVidia driver and the result
is the same, it fails to link even if that GS output is captured by TF.

My interpretation of the spec is that since GS outputs to stream 0 are
not passed down the pipeline they simply do not exist in the eyes of the
FS, that is, I see this situation as the same in which we declare an
input in the FS that is not declared as output in the GS. But it is true
that the spec does not address this situation explicitly, so I think
both interpretations could be valid.

I still think that failing to link is better though. If we report a link
failure the developer knows what is going on and the fix is trivial,
otherwise they will run into incorrect rendering, they will have to
figure out what is going and eventually fix the code anyway...

Since at least nVidia proprietary is failing to link as well in these
scenarios I guess our chances of running into shaders that we fail to
link for this reason and were expected to link properly are pretty small
too.

Iago