[Mesa-dev] [PATCH 3/3] glsl/linker: validate explicit locations for SSO programs

[Iago Toral]

On Fri, 2017-10-20 at 22:29 +1100, Timothy Arceri wrote:
> On 20/10/17 22:15, Iago Toral wrote:
> > On Fri, 2017-10-20 at 13:07 +0200, Iago Toral wrote:
> > > On Fri, 2017-10-20 at 22:03 +1100, Timothy Arceri wrote:
> > > > 
> > > > On 20/10/17 21:46, Iago Toral Quiroga wrote:
> > > > > ---
> > > > >    src/compiler/glsl/link_varyings.cpp | 53
> > > > > +++++++++++++++++++++++++++++++++++++
> > > > >    src/compiler/glsl/link_varyings.h   |  4 +++
> > > > >    src/compiler/glsl/linker.cpp        |  6 +++++
> > > > >    3 files changed, 63 insertions(+)
> > > > > 
> > > > > diff --git a/src/compiler/glsl/link_varyings.cpp
> > > > > b/src/compiler/glsl/link_varyings.cpp
> > > > > index dffb4f98df..d7e407184d 100644
> > > > > --- a/src/compiler/glsl/link_varyings.cpp
> > > > > +++ b/src/compiler/glsl/link_varyings.cpp
> > > > > @@ -599,6 +599,59 @@
> > > > > validate_explicit_variable_location(struct
> > > > > gl_context *ctx,
> > > > >    }
> > > > >    
> > > > >    /**
> > > > > + * Validate explicit locations for SSO programs. For non-SSO
> > > > > programs this
> > > > > + * is alreadty done in cross_validate_outputs_to_inputs().
> > > > > + */
> > > > > +void
> > > > > +validate_sso_explicit_locations(struct gl_context *ctx,
> > > > > +                                struct gl_shader_program
> > > > > *prog)
> > > > > +{
> > > > > +   assert(prog->SeparateShader);
> > > > > +   struct explicit_location_info
> > > > > explicit_locations_in[MAX_VARYINGS_INCL_PATCH][4];
> > > > > +   struct explicit_location_info
> > > > > explicit_locations_out[MAX_VARYINGS_INCL_PATCH][4];
> > > > > +
> > > > > +   for (unsigned i = 0; i <= MESA_SHADER_FRAGMENT; i++) {
> > > > > +      if (prog->_LinkedShaders[i] == NULL)
> > > > > +         continue;
> > > > > +
> > > > > +      gl_linked_shader *sh = prog->_LinkedShaders[i];
> > > > > +
> > > > > +      memset(explicit_locations_in, 0,
> > > > > sizeof(explicit_locations_in));
> > > > > +      memset(explicit_locations_out, 0,
> > > > > sizeof(explicit_locations_out));
> > > > > +
> > > > > +      foreach_in_list(ir_instruction, node, sh->ir) {
> > > > > +         ir_variable *const var = node->as_variable();
> > > > > +
> > > > > +         if (var == NULL ||
> > > > > +             !var->data.explicit_location ||
> > > > > +             var->data.location < VARYING_SLOT_VAR0 ||
> > > > > +             (var->data.mode != ir_var_shader_in &&
> > > > > +              var->data.mode != ir_var_shader_out))
> > > > > +            continue;
> > > > > +
> > > > > +         /* Vertex shader inputs and fragment shader outputs
> > > > > are
> > > > > validated
> > > > > +          * in assign_attribute_or_color_locations, skip
> > > > > them.
> > > > > +          */
> > > > > +         if (sh->Stage == MESA_SHADER_FRAGMENT &&
> > > > > +             var->data.mode == ir_var_shader_out)
> > > > > +            continue;
> > > > > +
> > > > > +         if (sh->Stage == MESA_SHADER_VERTEX &&
> > > > > +             var->data.mode == ir_var_shader_in)
> > > > > +            continue;
> > > > > +
> > > > > +         if (!validate_explicit_variable_location(
> > > > > +               ctx,
> > > > > +               var->data.mode == ir_var_shader_in ?
> > > > > explicit_locations_in :
> > > > > +                                                    explicit
> > > > > _loc
> > > > > at
> > > > > ions_out,
> > > > > +               var, prog, sh)) {
> > > > > +            return;
> > > > > +         }
> > > > > +      }
> > > > > +   }
> > > > > +}
> > > > > +
> > > > > +/**
> > > > >     * Validate that outputs from one stage match inputs of
> > > > > another
> > > > >     */
> > > > >    void
> > > > > diff --git a/src/compiler/glsl/link_varyings.h
> > > > > b/src/compiler/glsl/link_varyings.h
> > > > > index 081b04ea38..e8486208a2 100644
> > > > > --- a/src/compiler/glsl/link_varyings.h
> > > > > +++ b/src/compiler/glsl/link_varyings.h
> > > > > @@ -300,6 +300,10 @@ link_varyings(struct gl_shader_program
> > > > > *prog,
> > > > > unsigned first, unsigned last,
> > > > >                  struct gl_context *ctx, void *mem_ctx);
> > > > >    
> > > > >    void
> > > > > +validate_sso_explicit_locations(struct gl_context *ctx,
> > > > > +                                struct gl_shader_program
> > > > > *prog);
> > > > > +
> > > > > +void
> > > > >    cross_validate_outputs_to_inputs(struct gl_context *ctx,
> > > > >                                     struct gl_shader_program
> > > > > *prog,
> > > > >                                     gl_linked_shader
> > > > > *producer,
> > > > > diff --git a/src/compiler/glsl/linker.cpp
> > > > > b/src/compiler/glsl/linker.cpp
> > > > > index 3798309678..cba14f940b 100644
> > > > > --- a/src/compiler/glsl/linker.cpp
> > > > > +++ b/src/compiler/glsl/linker.cpp
> > > > > @@ -4938,6 +4938,12 @@ link_shaders(struct gl_context *ctx,
> > > > > struct
> > > > > gl_shader_program *prog)
> > > > >          prev = i;
> > > > >       }
> > > > >    
> > > > > +   /* Validate location aliasing for SSO programs (this is
> > > > > done
> > > > > during
> > > > > +    * cross_validate_outputs_to_inputs for non-SSO
> > > > > programs).
> > > > > +    */
> > > > > +   if (prog->SeparateShader)
> > > > > +      validate_sso_explicit_locations(ctx, prog);
> > > > 
> > > > Somewhere in link shaders we already calculate the first and
> > > > last
> > > > shader
> > > > stages. I think we should probably pass those to this function
> > > > and
> > > > then
> > > > only run the validation on the input of the first and output of
> > > > the
> > > > last, otherwise we are re-validating things the are already
> > > > covered
> > > > by
> > > > the non-sso checks.
> > > 
> > > Aha, good point, thanks. I'll look into that.
> > 
> > Actually, I think maybe I read your proposal too fast :). The only
> > place where we check for this for non-sso paths is
> > cross_validate_outputs_to_inputs() and we don't call that for SSO
> > programs, so there is no chance that we are re-validating anything
> > here. Am I missing something?
> 
> We do (or at least should) call cross_validate_outputs_to_inputs()
> for 
> SSO programs when they contain more than a single stage in a single 
> program. A single SSO program can consist of one or many stages.

Ah yes, you're right!

Iago

> > 
> > > Iago
> > > 
> > > > > +
> > > > >       /* Cross-validate uniform blocks between shader stages
> > > > > */
> > > > >       validate_interstage_uniform_blocks(prog, prog-
> > > > > > _LinkedShaders);
> > > > > 
> > > > >       if (!prog->data->LinkStatus)
> > > > > 
> 
>