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

[Timothy Arceri]

On Wed, 2016-08-24 at 15:12 -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.
> 
> Fixes piglit tests:
> 
>     oes_geometry_shader/sso_validation/user-defined-gs-input-in-block
>     oes_geometry_shader/sso_validation/user-defined-gs-input-not-in-
> block
> 
> v2: Don't apply none-smooth or smooth-none matching in desktop GL at
> link time.  Suggested by Timothy Arceri.
> 
> v3: Simplify the checks.  Only allow SMOOTH/NONE matching when the
> consumer stage is fragment.
> 
> 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 | 39
> +++++++++++++++++++++++++++++++++----
>  src/compiler/glsl/link_varyings.h   |  6 ++++++
>  src/mesa/main/shader_query.cpp      |  5 ++++-
>  4 files changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> index 581367b..19282ef 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -3023,17 +3023,6 @@ interpret_interpolation_qualifier(const struct
> ast_type_qualifier *qual,
>        interpolation = INTERP_MODE_NOPERSPECTIVE;
>     else if (qual->flags.q.smooth)
>        interpolation = INTERP_MODE_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_MODE_SMOOTH;
>     else
>        interpolation = INTERP_MODE_NONE;
>  
> diff --git a/src/compiler/glsl/link_varyings.cpp
> b/src/compiler/glsl/link_varyings.cpp
> index 1bce3e0..0831492 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -201,6 +201,34 @@ 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_mode producer_interp,
> +                         enum glsl_interp_mode consumer_interp)
> +{
> +   /* Section 4.5 (Interpolation Qualifiers) of the GLSL ES 3.20
> spec says:
> +    *
> +    *    When no interpolation qualifier is present, smooth
> interpolation is
> +    *    used.
> +    *
> +    * For the purpose of this function, use the ES rule even on
> desktop.  Link
> +    * validation for desktop will use a stronger check, but SSO IO
> validation
> +    * will use this check.  This allows using an undecorated output
> with a
> +    * "smooth" decorated input, for example.  There is no specific
> +    * justification for this in the OpenGL spec, but applications
> are known to
> +    * depend on this.

Are you sure this is ok? Previously we would require both to
be INTERP_MODE_NONE and the backend would change them to smooth later
unless glShadeModel() is set in which case they could be set to flat.

So now we will allow a case were one stage is NONE, one stage is SMOOTH
and glShadeModel() is flat. Previously this would fail validation, now
it would pass and give a user results they might not be expecting.

> +    */
> +   if (producer_interp == consumer_interp)
> +      return true;
> +
> +   return consumer_stage == MESA_SHADER_FRAGMENT &&
> +          ((producer_interp == INTERP_MODE_NONE &&
> +            consumer_interp == INTERP_MODE_SMOOTH) ||
> +           (producer_interp == INTERP_MODE_SMOOTH &&
> +            consumer_interp == INTERP_MODE_NONE));
> +}
> +
>  /**
>   * Validate the types and qualifiers of an output from one stage
> against the
>   * matching input to another stage.
> @@ -347,8 +375,12 @@ 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->IsES &&
> +        !interpolation_compatible(producer_stage, consumer_stage,
> +                                  glsl_interp_mode(output-
> >data.interpolation),
> +                                  glsl_interp_mode(input-
> >data.interpolation))) ||
> +       (!prog->IsES && prog->Version < 440 &&
> +        input->data.interpolation != output->data.interpolation)) {
>        linker_error(prog,
>                     "%s shader output `%s' specifies %s "
>                     "interpolation qualifier, "
> @@ -1390,8 +1422,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) {


If we no longer set consumer here then you should also remove the if
(consumer_var) { ... } below as it will now never be called.



>        /* 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 afce56e..075bb72 100644
> --- a/src/compiler/glsl/link_varyings.h
> +++ b/src/compiler/glsl/link_varyings.h
> @@ -341,4 +341,10 @@ check_against_input_limit(struct gl_context
> *ctx,
>                            gl_linked_shader *consumer,
>                            unsigned num_explicit_locations);
>  
> +bool
> +interpolation_compatible(gl_shader_stage producer_stage,
> +                         gl_shader_stage consumer_stage,
> +                         enum glsl_interp_mode producer_interp,
> +                         enum glsl_interp_mode consumer_interp);
> +
>  #endif /* GLSL_LINK_VARYINGS_H */
> diff --git a/src/mesa/main/shader_query.cpp
> b/src/mesa/main/shader_query.cpp
> index 0eae39a..a0c764a 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"
>  
> @@ -1611,7 +1612,9 @@ validate_io(struct gl_shader_program *producer,
>           }
>        }
>  
> -      if (producer_var->interpolation != consumer_var-
> >interpolation) {
> +      if (!interpolation_compatible(producer_stage, consumer_stage,
> +                                    glsl_interp_mode(producer_var-
> >interpolation),
> +                                    glsl_interp_mode(consumer_var-
> >interpolation))) {
>           valid = false;
>           goto out;
>        }