[Mesa-dev] [PATCH 05/11] glsl: Don't monkey about with the interpolation modes

[Timothy Arceri]

On Tue, 2016-06-14 at 19:01 -0700, Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
> 
> Previously we'd munge the interpolation mode so that later checks in
> the
> GLSL linker would pass.  The caused problems for similar checks in
> SSO
> IO validation.  Instead, make the check smarter, use the same check
> in
> both places, and don't modify the interpolation mode.
> 
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96358
> Cc: "12.0" <mesa-stable at lists.freedesktop.org>
> Cc: Gregory Hainaut <gregory.hainaut at gmail.com>
> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
>  src/compiler/glsl/ast_to_hir.cpp    | 11 ----------
>  src/compiler/glsl/link_varyings.cpp | 41
> +++++++++++++++++++++++++++++++++----
>  src/compiler/glsl/link_varyings.h   |  7 +++++++
>  src/mesa/main/shader_query.cpp      |  6 +++++-
>  4 files changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> index 7da734c..d675dfa 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -2991,17 +2991,6 @@ interpret_interpolation_qualifier(const struct
> ast_type_qualifier *qual,
>        interpolation = INTERP_QUALIFIER_NOPERSPECTIVE;
>     else if (qual->flags.q.smooth)
>        interpolation = INTERP_QUALIFIER_SMOOTH;
> -   else if (state->es_shader &&
> -            ((mode == ir_var_shader_in &&
> -              state->stage != MESA_SHADER_VERTEX) ||
> -             (mode == ir_var_shader_out &&
> -              state->stage != MESA_SHADER_FRAGMENT)))
> -      /* Section 4.3.9 (Interpolation) of the GLSL ES 3.00 spec
> says:
> -       *
> -       *    "When no interpolation qualifier is present, smooth
> interpolation
> -       *    is used."
> -       */
> -      interpolation = INTERP_QUALIFIER_SMOOTH;
>     else
>        interpolation = INTERP_QUALIFIER_NONE;
>  
> diff --git a/src/compiler/glsl/link_varyings.cpp
> b/src/compiler/glsl/link_varyings.cpp
> index 534393a..54491fc 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -201,6 +201,37 @@ anonymous_struct_type_matches(const glsl_type
> *output_type,
>             to_match->record_compare(output_type);
>  }
>  
> +bool
> +interpolation_compatible(gl_shader_stage producer_stage,
> +                         gl_shader_stage consumer_stage,
> +                         enum glsl_interp_qualifier producer_interp,
> +                         enum glsl_interp_qualifier consumer_interp,
> +                         bool is_builtin_variable)
> +{
> +   if (producer_interp == consumer_interp)
> +      return true;
> +
> +   if (is_builtin_variable)
> +      return false;
> +
> +   /* Section 4.3.9 (Interpolation) of the GLSL ES 3.00 spec says:
> +    *
> +    *    When no interpolation qualifier is present, smooth
> interpolation is
> +    *    used.
> +    */

Note last time I was looking at this I couldn't find this text in the
desktop spec so I don't think the following code can be applied to
desktop gl.

> +   if (producer_stage == MESA_SHADER_VERTEX &&
> +       producer_interp == INTERP_QUALIFIER_NONE &&
> +       consumer_interp == INTERP_QUALIFIER_SMOOTH)
> +      return true;
> +
> +   if (consumer_stage == MESA_SHADER_FRAGMENT &&
> +       consumer_interp == INTERP_QUALIFIER_NONE &&
> +       producer_interp == INTERP_QUALIFIER_SMOOTH)
> +      return true;

Are you sure this is enough? What about a fragment shader with smooth
and a geom shader with none? That shouldn't that return true also?

> +
> +   return false;
> +}
> +
>  /**
>   * Validate the types and qualifiers of an output from one stage
> against the
>   * matching input to another stage.
> @@ -329,8 +360,11 @@ cross_validate_types_and_qualifiers(struct
> gl_shader_program *prog,
>      *     qualifiers of variables of the same name do not match.
>      *
>      */
> -   if (input->data.interpolation != output->data.interpolation &&
> -       prog->Version < 440) {
> +   if (prog->Version < 440 &&
> +       !interpolation_compatible(producer_stage, consumer_stage,
> +                                 glsl_interp_qualifier(output-
> >data.interpolation),
> +                                 glsl_interp_qualifier(input-
> >data.interpolation),
> +                                 is_gl_identifier(output->name))) {
>        linker_error(prog,
>                     "%s shader output `%s' specifies %s "
>                     "interpolation qualifier, "
> @@ -1371,8 +1405,7 @@ varying_matches::record(ir_variable
> *producer_var, ir_variable *consumer_var)
>        (producer_var->type->contains_integer() ||
>         producer_var->type->contains_double());
>  
> -   if (needs_flat_qualifier ||
> -       (consumer_stage != -1 && consumer_stage !=
> MESA_SHADER_FRAGMENT)) {
> +   if (needs_flat_qualifier) {
>        /* Since this varying is not being consumed by the fragment
> shader, its
>         * interpolation type varying cannot possibly affect
> rendering.
>         * Also, this variable is non-flat and is (or contains) an
> integer
> diff --git a/src/compiler/glsl/link_varyings.h
> b/src/compiler/glsl/link_varyings.h
> index 39e9070..6a98c0f 100644
> --- a/src/compiler/glsl/link_varyings.h
> +++ b/src/compiler/glsl/link_varyings.h
> @@ -338,4 +338,11 @@ check_against_input_limit(struct gl_context
> *ctx,
>                            gl_shader *consumer,
>                            unsigned num_explicit_locations);
>  
> +bool
> +interpolation_compatible(gl_shader_stage producer_stage,
> +                         gl_shader_stage consumer_stage,
> +                         enum glsl_interp_qualifier producer_interp,
> +                         enum glsl_interp_qualifier consumer_interp,
> +                         bool is_builtin_variable);
> +
>  #endif /* GLSL_LINK_VARYINGS_H */
> diff --git a/src/mesa/main/shader_query.cpp
> b/src/mesa/main/shader_query.cpp
> index b2e53fb..4255bf2 100644
> --- a/src/mesa/main/shader_query.cpp
> +++ b/src/mesa/main/shader_query.cpp
> @@ -37,6 +37,7 @@
>  #include "compiler/glsl/glsl_symbol_table.h"
>  #include "compiler/glsl/ir.h"
>  #include "compiler/glsl/program.h"
> +#include "compiler/glsl/link_varyings.h"
>  #include "program/hash_table.h"
>  #include "util/strndup.h"
>  
> @@ -1610,7 +1611,10 @@ validate_io(struct gl_shader_program
> *producer,
>           }
>        }
>  
> -      if (producer_var->interpolation != consumer_var-
> >interpolation) {
> +      if (!interpolation_compatible(producer_stage, consumer_stage,
> +                                    glsl_interp_qualifier(producer_v
> ar->interpolation),
> +                                    glsl_interp_qualifier(consumer_v
> ar->interpolation),
> +                                    is_gl_identifier(consumer_var-
> >name))) {
>           valid = false;
>           goto out;
>        }