[Mesa-dev] [PATCH 4/6] glsl/linker: don't fail non static used inputs without matching outputs

[Andres Gomez]

On Wed, 2019-02-06 at 09:42 +1100, Timothy Arceri wrote:
> On 6/2/19 1:11 am, Andres Gomez wrote:
> > On Fri, 2019-02-01 at 18:37 -0500, Ilia Mirkin wrote:
> > > On Fri, Feb 1, 2019 at 1:08 PM Andres Gomez <agomez at igalia.com> wrote:
> > > > If there is no Static Use of an input variable, the linker shouldn't
> > > > fail whenever there is no defined matching output variable in the
> > > > previous stage.
> > > > 
> > > >  From page 47 (page 51 of the PDF) of the GLSL 4.60 v.5 spec:
> > > > 
> > > >    " Only the input variables that are statically read need to be
> > > >      written by the previous stage; it is allowed to have superfluous
> > > >      declarations of input variables."
> > > > 
> > > > Now, we complete this exception whenever the input variable has an
> > > > explicit location. Previously, 18004c338f6 ("glsl: fail when a
> > > > shader's input var has not an equivalent out var in previous") took
> > > > care of the cases in which the input variable didn't have an explicit
> > > > location.
> > > > 
> > > > Additionally, likewise 1aa5738e666 ("glsl: relax input->output
> > > > validation for SSO programs"), avoid failing also for programs that
> > > > utilize GL_ARB_separate_shader_objects.
> > > > 
> > > > Cc: Timothy Arceri <tarceri at itsqueeze.com>
> > > > Cc: Iago Toral Quiroga <itoral at igalia.com>
> > > > Cc: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> > > > Cc: Tapani Pälli <tapani.palli at intel.com>
> > > > Cc: Ian Romanick <ian.d.romanick at intel.com>
> > > > Signed-off-by: Andres Gomez <agomez at igalia.com>
> > > > ---
> > > >   src/compiler/glsl/link_varyings.cpp | 16 ++++++++++++++--
> > > >   1 file changed, 14 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> > > > index e5f7d3e322a..6cebc5b3c5a 100644
> > > > --- a/src/compiler/glsl/link_varyings.cpp
> > > > +++ b/src/compiler/glsl/link_varyings.cpp
> > > > @@ -808,8 +808,20 @@ cross_validate_outputs_to_inputs(struct gl_context *ctx,
> > > > 
> > > >                  output = output_explicit_locations[idx][input->data.location_frac].var;
> > > > 
> > > > -               if (output == NULL ||
> > > > -                   input->data.location != output->data.location) {
> > > > +               if (output == NULL) {
> > > > +                  /* A linker failure should only happen when, for programs
> > > > +                   * not using sso, there is no output declaration and there
> > > > +                   * is Static Use of the declared input.
> > > > +                   */
> > > > +                  if (input->data.used && !prog->SeparateShader) {
> > > 
> > > Should this differentiate whether this is the first stage of a
> > > separable program vs a later one? Presumably the exception only
> > > applies at the separate program boundary, not to each shader within
> > > the program?
> > 
> > Cc-ing Tapani and Anuj.
> > 
> > As I understand the spec, it applies to every shader within the
> > separable program.
> 
> No that's not what the spec says. From the quote below:
> 
> "With separable program objects, interfaces between shader stages 
> may involve the outputs from one program object and the inputs from a 
> second program object."
> 
> So if we have two "program" objects say:
> 
> 1. vs->gs
> 2. fs
> 
> The relaxed restrictions only apply to the gs -> fs interface. Not the 
> vs -> gs interface.

After double checking specs and examples, I think you are right.

I'll update the patch and send a v2 shortly.

> 
> 
> >  From the ARB_separate_shader_objects spec:
> > 
> >    " With separable program objects, interfaces between shader stages
> >      may involve the outputs from one program object and the inputs
> >      from a second program object.  For such interfaces, it is not
> >      possible to detect mismatches at link time, because the programs
> >      are linked separately.  When each such program is linked, all
> >      inputs or outputs interfacing with another program stage are
> >      treated as active.  The linker will generate an executable that
> >      assumes the presence of a compatible program on the other side of
> >      the interface.  If a mismatch between programs occurs, no GL error
> >      will be generated, but some or all of the inputs on the interface
> >      will be undefined."
> > 
> > Notice that this has also been the interpretation from:
> > 
> > 1aa5738e666 ("glsl: relax input->output validation for SSO programs")
> 
> Yes this looks wrong also. We should write some piglit tests for this 
> and fix it too.

I'll address reverting that commit later.

Doing so makes "arb_program_interface_query-getprogramresourceiv" and
"arb_program_interface_query-resource-query" piglit tests fail so it
seems I'll have to fix those tests and add some more to check the
interface matching in the internal interfaces of separable programs.

> 
> > > > +                     linker_error(prog,
> > > > +                                  "%s shader input `%s' with explicit location "
> > > > +                                  "has no matching output\n",
> > > > +                                  _mesa_shader_stage_to_string(consumer->Stage),
> > > > +                                  input->name);
> > > > +                     break;
> > > > +                  }
> > > > +               } else if (input->data.location != output->data.location) {
> > > >                     linker_error(prog,
> > > >                                  "%s shader input `%s' with explicit location "
> > > >                                  "has no matching output\n",
> > > > --
> > > > 2.20.1
> > > > 
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-- 
Br,

Andres