[Mesa-dev] [PATCH] glsl: for anonymous struct matching use without_array() (v3)

[Ilia Mirkin]

On Sun, Jun 5, 2016 at 9:30 PM, Dave Airlie <airlied at gmail.com> wrote:
> From: Dave Airlie <airlied at redhat.com>
>
> With tessellation shaders we can have cases where we have
> arrays of anon structs, so make sure we match using without_array().
>
> Fixes:
> GL45-CTS.tessellation_shader.tessellation_control_to_tessellation_evaluation.gl_in
>
> v2:
> test lengths match as well (Ilia)
> v3:
> descend array lengths to check for matches as well (Ilia)
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>

This looks good, but I have some petty suggestions which you're free
to incorporate, or not.

> ---
>  src/compiler/glsl/link_varyings.cpp | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> index a286e77..1392095 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -182,6 +182,31 @@ process_xfb_layout_qualifiers(void *mem_ctx, const gl_shader *sh,
>     return has_xfb_qualifiers;
>  }
>
> +static bool
> +anonymous_struct_type_matches(const glsl_type *output_type,
> +                              const glsl_type *to_match)
> +{
> +    const glsl_type *output_tmp = output_type;
> +    const glsl_type *to_match_tmp = to_match;

I'd personally not even have the _tmp vars, just use the args.

> +
> +    while (output_tmp) {
> +        if (output_tmp->is_array() && to_match_tmp->is_array()) {

You could just do

while (output_tmp->is_array() && .. ) {

I don't think output_tmp == null can ever happen.

> +            /* if the lengths at each level don't match fail. */
> +            if (output_tmp->length != to_match_tmp->length)
> +                return false;
> +            output_tmp = output_tmp->fields.array;
> +            to_match_tmp = to_match_tmp->fields.array;
> +        } else {

And then this would be outside of the while loop. IMHO it closer
matches the flow of the code. Your call. Either way,

Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>

> +            if (output_tmp->is_array() || to_match_tmp->is_array())
> +                return false;
> +            return output_tmp->is_anonymous() &&
> +                   to_match_tmp->is_anonymous() &&
> +                   to_match_tmp->record_compare(output_tmp);
> +        }
> +    }
> +    return false;
> +}
> +
>  /**
>   * Validate the types and qualifiers of an output from one stage against the
>   * matching input to another stage.
> @@ -226,9 +251,7 @@ cross_validate_types_and_qualifiers(struct gl_shader_program *prog,
>         *     fragment language."
>         */
>        if (!output->type->is_array() || !is_gl_identifier(output->name)) {
> -         bool anon_matches = output->type->is_anonymous() &&
> -            type_to_match->is_anonymous() &&
> -            type_to_match->record_compare(output->type);
> +         bool anon_matches = anonymous_struct_type_matches(output->type, type_to_match);
>
>           if (!anon_matches) {
>              linker_error(prog,
> --
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev