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

[Iago Toral]

I sent patches 9 and 10 on top of the series to address the comments
you had. I think this makes sense since at least some of them relate to
pre-existing code one of them refactors the loops used in the checks
and that would be a bit annoying to do in the middle of the series, I
hope that that's okay.

Iago

On Tue, 2017-10-24 at 23:47 -0400, Ilia Mirkin wrote:
> For all the patches in the series that I didn't have comments on,
> 
> Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>
> 
> On Tue, Oct 24, 2017 at 5:28 AM, Iago Toral Quiroga <itoral at igalia.co
> m> wrote:
> > v2:
> > - we only need to validate inputs to the first stage and outputs
> >   from the last stage, everything else has already been validated
> >   during cross_validate_outputs_to_inputs (Timothy).
> > - Use MAX_VARYING instead of MAX_VARYINGS_INCL_PATCH (Illia)
> > ---
> >  src/compiler/glsl/link_varyings.cpp | 55
> > +++++++++++++++++++++++++++++++++++++
> >  src/compiler/glsl/link_varyings.h   |  6 ++++
> >  src/compiler/glsl/linker.cpp        | 10 +++++++
> >  3 files changed, 71 insertions(+)
> > 
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index 02a48f7199..e80736fb38 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -600,6 +600,61 @@ validate_explicit_variable_location(struct
> > gl_context *ctx,
> >  }
> > 
> >  /**
> > + * Validate explicit locations for the inputs to the first stage
> > and the
> > + * outputs of the last stage in an SSO program (everything in
> > between is
> > + * validated in cross_validate_outputs_to_inputs).
> > + */
> > +void
> > +validate_sso_explicit_locations(struct gl_context *ctx,
> > +                                struct gl_shader_program *prog,
> > +                                gl_shader_stage first_stage,
> > +                                gl_shader_stage last_stage)
> > +{
> > +   assert(prog->SeparateShader);
> > +
> > +   /* VS inputs and FS outputs are validated in
> > +    * assign_attribute_or_color_locations()
> > +    */
> > +   bool validate_first_stage = first_stage != MESA_SHADER_VERTEX;
> > +   bool validate_last_stage = last_stage != MESA_SHADER_FRAGMENT;
> > +   if (!validate_first_stage && !validate_last_stage)
> > +      return;
> > +
> > +   struct explicit_location_info
> > explicit_locations[MAX_VARYING][4];
> > +
> > +   gl_shader_stage stages[2] = { first_stage, last_stage };
> > +   bool validate_stage[2] = { validate_first_stage,
> > validate_last_stage };
> > +   ir_variable_mode var_direction[2] = { ir_var_shader_in,
> > ir_var_shader_out };
> > +
> > +   for (unsigned i = 0; i < 2; i++) {
> > +      if (!validate_stage[i])
> > +         continue;
> > +
> > +      gl_shader_stage stage = stages[i];
> > +
> > +      gl_linked_shader *sh = prog->_LinkedShaders[stage];
> > +      assert(sh);
> > +
> > +      memset(explicit_locations, 0, sizeof(explicit_locations));
> > +
> > +      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 != var_direction[i])
> > +            continue;
> > +
> > +         if (!validate_explicit_variable_location(
> > +               ctx, explicit_locations, 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..e052a2b3e5 100644
> > --- a/src/compiler/glsl/link_varyings.h
> > +++ b/src/compiler/glsl/link_varyings.h
> > @@ -300,6 +300,12 @@ 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,
> > +                                gl_shader_stage first,
> > +                                gl_shader_stage last);
> > +
> > +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..8c610df9bf 100644
> > --- a/src/compiler/glsl/linker.cpp
> > +++ b/src/compiler/glsl/linker.cpp
> > @@ -4938,6 +4938,16 @@ link_shaders(struct gl_context *ctx, struct
> > gl_shader_program *prog)
> >        prev = i;
> >     }
> > 
> > +   /* The cross validation of outputs/inputs above validates
> > explicit locations
> > +    * but for SSO programs we need to do this also for the inputs
> > in the
> > +    * first stage and outputs of the last stage included in the
> > program, since
> > +    * there is no cross validation for these.
> > +    */
> > +   if (prog->SeparateShader)
> > +      validate_sso_explicit_locations(ctx, prog,
> > +                                      (gl_shader_stage) first,
> > +                                      (gl_shader_stage) last);
> > +
> >     /* Cross-validate uniform blocks between shader stages */
> >     validate_interstage_uniform_blocks(prog, prog->_LinkedShaders);
> >     if (!prog->data->LinkStatus)
> > --
> > 2.11.0
> > 
> 
>