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

[Timothy Arceri]

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.

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