[Mesa-dev] [PATCH 14/22] nir/linker: add some cross stage uniform validation

[Alejandro Piñeiro]

We found some issues with this patch, so we are dropping it. No need to
review it. We will send a new patch when we finish that, but it is not
needed to stop the review of the rest of the patches of the series for
that. This patch was already independent enough.

If someone is interested on the details: this patch was trying to cross
stage a incoming nir variable uniform, and discard it if it was already
processed. But processed as a gl_uniform_storage entry. As this dropped
patch shows, comparing both was already complicating things on the
simple cases, and we found that was not working at all with structs. So
the new approach would be do a previous cross-stage validation,
comparing a nir variable against another nir variable.

BR


On 17/04/18 16:36, Alejandro Piñeiro wrote:
> For now it checks that the type and array size of the uniforms is the
> same, as right now only this and uniform->opaque is filled (and this
> is specific for each stage). More checks will be added as more
> features get added.
>
> We are not checking for the same explicit location as we are basing
> the linking on the location.
>
> We are also avoiding name-checks. As they are just debug info, nothing
> prevents to have one SPIR-V module with the names and other without
> it. It also seems reasonable to think that it is not even needed to
> keep the same names on different modules/stages.
>
> As mentioned on the comment included with this patch, on GLSL linker
> there are some full cross-stage validation methods, taking into
> account more than just the uniforms. It is likely that this gets moved
> later to a bigger method like that.
> ---
>  src/compiler/nir/nir_link_uniforms.c | 68 +++++++++++++++++++++++++++++++-----
>  1 file changed, 60 insertions(+), 8 deletions(-)
>
> diff --git a/src/compiler/nir/nir_link_uniforms.c b/src/compiler/nir/nir_link_uniforms.c
> index af01a83a8e3..754385967f6 100644
> --- a/src/compiler/nir/nir_link_uniforms.c
> +++ b/src/compiler/nir/nir_link_uniforms.c
> @@ -215,6 +215,21 @@ get_next_index(struct nir_link_uniforms_state *state,
>     return index;
>  }
>  
> +static void
> +handle_type_for_uniform_storage(const struct glsl_type *original_type,
> +                                const struct glsl_type **storage_type,
> +                                unsigned *array_elements)
> +{
> +
> +   const struct glsl_type *type_no_array = glsl_without_array(original_type);
> +   if (glsl_type_is_array(original_type)) {
> +      *storage_type = type_no_array;
> +      *array_elements = glsl_get_length(original_type);
> +   } else {
> +      *storage_type = original_type;
> +      *array_elements = 0;
> +   }
> +}
>  
>  /**
>   * Creates the neccessary entries in UniformStorage for the uniform. Returns
> @@ -292,14 +307,8 @@ nir_link_uniform(struct gl_context *ctx,
>         */
>        uniform->name = NULL;
>  
> -      const struct glsl_type *type_no_array = glsl_without_array(type);
> -      if (glsl_type_is_array(type)) {
> -         uniform->type = type_no_array;
> -         uniform->array_elements = glsl_get_length(type);
> -      } else {
> -         uniform->type = type;
> -         uniform->array_elements = 0;
> -      }
> +      handle_type_for_uniform_storage(type, &uniform->type,
> +                                      &uniform->array_elements);
>        uniform->active_shader_mask |= 1 << stage;
>  
>        /* Uniform has an explicit location */
> @@ -327,6 +336,7 @@ nir_link_uniform(struct gl_context *ctx,
>  
>        unsigned entries = MAX2(1, uniform->array_elements);
>  
> +      const struct glsl_type *type_no_array = glsl_without_array(type);
>        if (glsl_type_is_sampler(type_no_array)) {
>           int sampler_index =
>              get_next_index(state, uniform, &state->next_sampler_index);
> @@ -383,6 +393,45 @@ nir_link_uniform(struct gl_context *ctx,
>     }
>  }
>  
> +/*
> + * Validate if the uniform already linked before on @existing is compatible
> + * with the new usage of it on another stage at @current.
> + *
> + * GLSL linker has specific methods for validation after the linkage, so
> + * probably this bit will be moved later to a more general one.
> + */
> +static bool
> +cross_validate_uniform(struct gl_shader_program *prog,
> +                       struct gl_uniform_storage* existing,
> +                       nir_variable *current)
> +{
> +   const struct glsl_type *type = current->type;
> +   unsigned array_elements = 0;
> +
> +   handle_type_for_uniform_storage(current->type, &type, &array_elements);
> +
> +   if (existing->type != type) {
> +      linker_error(prog, "uniform with location %i declared as "
> +                   "type `%s' and type `%s'\n", current->data.location,
> +                   glsl_get_type_name(type),
> +                   glsl_get_type_name(existing->type));
> +
> +      return false;
> +   }
> +
> +   if (existing->array_elements != array_elements) {
> +      linker_error(prog, "uniform with location %i declared as "
> +                   "type `%s[%i]' and type `%s[%i]'\n", current->data.location,
> +                   glsl_get_type_name(type), array_elements,
> +                   glsl_get_type_name(existing->type), existing->array_elements);
> +
> +      return false;
> +   }
> +
> +   return true;
> +}
> +
> +
>  bool
>  nir_link_uniforms(struct gl_context *ctx,
>                    struct gl_shader_program *prog)
> @@ -418,6 +467,9 @@ nir_link_uniforms(struct gl_context *ctx,
>            */
>           uniform = find_previous_uniform_storage(prog, var->data.location);
>           if (uniform) {
> +            if (!cross_validate_uniform(prog, uniform, var))
> +               return false;
> +
>              uniform->active_shader_mask |= 1 << shader_type;
>  
>              continue;