[Mesa-dev] [PATCH] glsl: Checks for interpolation into its own function.

Timothy Arceri timothy.arceri at collabora.com
Mon Apr 18 22:53:32 UTC 2016


On Mon, 2016-04-18 at 19:44 +0300, Andres Gomez wrote:
> Hi,
> 
> I would really appreciate if you could find some time to review this
> patch.

Is there a patch somewhere that makes use of this change? 

> 
> Thanks!
> 
> On Mon, 2016-04-04 at 19:50 +0300, Andres Gomez wrote:
> > 
> > This generalizes the validation also to be done for variables
> > inside
> > interface blocks, which, for some cases, was missing.
> > 
> > For a discussion about the additional validation cases included see
> > https://lists.freedesktop.org/archives/mesa-dev/2016-March/109117.h
> > tm
> > l
> > and Khronos bug #15671.
> > 
> > Signed-off-by: Andres Gomez <agomez at igalia.com>
> > ---
> >  src/compiler/glsl/ast_to_hir.cpp | 316 +++++++++++++++++++++------
> > ------------
> >  1 file changed, 171 insertions(+), 145 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/ast_to_hir.cpp
> > b/src/compiler/glsl/ast_to_hir.cpp
> > index 7c9be81..e4ebc6b 100644
> > --- a/src/compiler/glsl/ast_to_hir.cpp
> > +++ b/src/compiler/glsl/ast_to_hir.cpp
> > @@ -2792,8 +2792,164 @@ apply_explicit_binding(struct
> > _mesa_glsl_parse_state *state,
> >  }
> >  
> >  
> > +static void
> > +validate_interpolation_qualifier(struct _mesa_glsl_parse_state
> > *state,
> > +                                 YYLTYPE *loc,
> > +                                 const glsl_interp_qualifier
> > interpolation,
> > +                                 const struct ast_type_qualifier
> > *qual,
> > +                                 const struct glsl_type *var_type,
> > +                                 ir_variable_mode mode)
> > +{
> > +   /* Interpolation qualifiers can only apply to shader inputs or
> > outputs, but
> > +    * not to vertex shader inputs nor fragment shader outputs.
> > +    *
> > +    * From section 4.3 ("Storage Qualifiers") of the GLSL 1.30
> > spec:
> > +    *    "Outputs from a vertex shader (out) and inputs to a
> > fragment
> > +    *    shader (in) can be further qualified with one or more of
> > these
> > +    *    interpolation qualifiers"
> > +    *    ...
> > +    *    "These interpolation qualifiers may only precede the
> > qualifiers in,
> > +    *    centroid in, out, or centroid out in a declaration. They
> > do
> > not apply
> > +    *    to the deprecated storage qualifiers varying or centroid
> > +    *    varying. They also do not apply to inputs into a vertex
> > shader or
> > +    *    outputs from a fragment shader."
> > +    *
> > +    * From section 4.3 ("Storage Qualifiers") of the GLSL ES 3.00
> > spec:
> > +    *    "Outputs from a shader (out) and inputs to a shader (in)
> > can be
> > +    *    further qualified with one of these interpolation
> > qualifiers."
> > +    *    ...
> > +    *    "These interpolation qualifiers may only precede the
> > qualifiers
> > +    *    in, centroid in, out, or centroid out in a declaration.
> > They do
> > +    *    not apply to inputs into a vertex shader or outputs from
> > a
> > +    *    fragment shader."
> > +    */
> > +   if (state->is_version(130, 300)
> > +       && interpolation != INTERP_QUALIFIER_NONE) {
> > +      const char *i = interpolation_string(interpolation);
> > +      if (mode != ir_var_shader_in && mode != ir_var_shader_out)
> > +         _mesa_glsl_error(loc, state,
> > +                          "interpolation qualifier `%s' can only
> > be
> > applied to "
> > +                          "shader inputs or outputs.", i);
> > +
> > +      switch (state->stage) {
> > +      case MESA_SHADER_VERTEX:
> > +         if (mode == ir_var_shader_in) {
> > +            _mesa_glsl_error(loc, state,
> > +                             "interpolation qualifier '%s' cannot
> > be
> > applied to "
> > +                             "vertex shader inputs", i);
> > +         }
> > +         break;
> > +      case MESA_SHADER_FRAGMENT:
> > +         if (mode == ir_var_shader_out) {
> > +            _mesa_glsl_error(loc, state,
> > +                             "interpolation qualifier '%s' cannot
> > be
> > applied to "
> > +                             "fragment shader outputs", i);
> > +         }
> > +         break;
> > +      default:
> > +         break;
> > +      }
> > +   }
> > +
> > +   /* Interpolation qualifiers cannot be applied to 'centroid' and
> > +    * 'centroid varying'.
> > +    *
> > +    * From section 4.3 ("Storage Qualifiers") of the GLSL 1.30
> > spec:
> > +    *    "interpolation qualifiers may only precede the qualifiers
> > in,
> > +    *    centroid in, out, or centroid out in a declaration. They
> > do
> > not apply
> > +    *    to the deprecated storage qualifiers varying or centroid
> > varying."
> > +    *
> > +    * These deprecated storage qualifiers do not exist in GLSL ES
> > 3.00.
> > +    */
> > +   if (state->is_version(130, 0)
> > +       && interpolation != INTERP_QUALIFIER_NONE
> > +       && qual->flags.q.varying) {
> > +
> > +      const char *i = interpolation_string(interpolation);
> > +      const char *s;
> > +      if (qual->flags.q.centroid)
> > +         s = "centroid varying";
> > +      else
> > +         s = "varying";
> > +
> > +      _mesa_glsl_error(loc, state,
> > +                       "qualifier '%s' cannot be applied to the "
> > +                       "deprecated storage qualifier '%s'", i, s);
> > +   }
> > +
> > +   /* Integer fragment inputs must be qualified with 'flat'.  In
> > GLSL ES,
> > +    * so must integer vertex outputs.
> > +    *
> > +    * From section 4.3.4 ("Inputs") of the GLSL 1.50 spec:
> > +    *    "Fragment shader inputs that are signed or unsigned
> > integers or
> > +    *    integer vectors must be qualified with the interpolation
> > qualifier
> > +    *    flat."
> > +    *
> > +    * From section 4.3.4 ("Input Variables") of the GLSL 3.00 ES
> > spec:
> > +    *    "Fragment shader inputs that are, or contain, signed or
> > unsigned
> > +    *    integers or integer vectors must be qualified with the
> > +    *    interpolation qualifier flat."
> > +    *
> > +    * From section 4.3.6 ("Output Variables") of the GLSL 3.00 ES
> > spec:
> > +    *    "Vertex shader outputs that are, or contain, signed or
> > unsigned
> > +    *    integers or integer vectors must be qualified with the
> > +    *    interpolation qualifier flat."
> > +    *
> > +    * Note that prior to GLSL 1.50, this requirement applied to
> > vertex
> > +    * outputs rather than fragment inputs.  That creates problems
> > in
> > the
> > +    * presence of geometry shaders, so we adopt the GLSL 1.50 rule
> > for all
> > +    * desktop GL shaders.  For GLSL ES shaders, we follow the spec
> > and
> > +    * apply the restriction to both vertex outputs and fragment
> > inputs.
> > +    *
> > +    * Note also that the desktop GLSL specs are missing the text
> > "or
> > +    * contain"; this is presumably an oversight, since there is no
> > +    * reasonable way to interpolate a fragment shader input that
> > contains
> > +    * an integer. See Khronos bug #15671.
> > +    */
> > +   if (state->is_version(130, 300)
> > +       && var_type->contains_integer()
> > +       && interpolation != INTERP_QUALIFIER_FLAT
> > +       && ((state->stage == MESA_SHADER_FRAGMENT && mode ==
> > ir_var_shader_in)
> > +           || (state->stage == MESA_SHADER_VERTEX && mode ==
> > ir_var_shader_out
> > +               && state->es_shader))) {
> > +      const char *shader_var_type = (state->stage ==
> > MESA_SHADER_VERTEX) ?
> > +         "vertex output" : "fragment input";
> > +      _mesa_glsl_error(loc, state, "if a %s is (or contains) "
> > +                       "an integer, then it must be qualified with
> > 'flat'",
> > +                       shader_var_type);
> > +   }
> > +
> > +   /* Double fragment inputs must be qualified with 'flat'.
> > +    *
> > +    * From the "Overview" of the ARB_gpu_shader_fp64 extension
> > spec:
> > +    *    "This extension does not support interpolation of double-
> > precision
> > +    *    values; doubles used as fragment shader inputs must be
> > qualified as
> > +    *    "flat"."
> > +    *
> > +    * From section 4.3.4 ("Inputs") of the GLSL 4.00 spec:
> > +    *    "Fragment shader inputs that are signed or unsigned
> > integers, integer
> > +    *    vectors, or any double-precision floating-point type must
> > be
> > +    *    qualified with the interpolation qualifier flat."
> > +    *
> > +    * Note that the GLSL specs are missing the text "or contain";
> > this is
> > +    * presumably an oversight. See Khronos bug #15671.
> > +    *
> > +    * The 'double' type does not exist in GLSL ES so far.
> > +    */
> > +   if ((state->ARB_gpu_shader_fp64_enable
> > +        || state->is_version(400, 0))
> > +       && var_type->contains_double()
> > +       && interpolation != INTERP_QUALIFIER_FLAT
> > +       && state->stage == MESA_SHADER_FRAGMENT
> > +       && mode == ir_var_shader_in) {
> > +      _mesa_glsl_error(loc, state, "if a fragment input is (or
> > contains) "
> > +                       "a double, then it must be qualified with
> > 'flat'");
> > +   }
> > +}
> > +
> >  static glsl_interp_qualifier
> >  interpret_interpolation_qualifier(const struct ast_type_qualifier
> > *qual,
> > +                                  const struct glsl_type
> > *var_type,
> >                                    ir_variable_mode mode,
> >                                    struct _mesa_glsl_parse_state
> > *state,
> >                                    YYLTYPE *loc)
> > @@ -2805,37 +2961,23 @@ interpret_interpolation_qualifier(const
> > struct ast_type_qualifier *qual,
> >        interpolation = INTERP_QUALIFIER_NOPERSPECTIVE;
> >     else if (qual->flags.q.smooth)
> >        interpolation = INTERP_QUALIFIER_SMOOTH;
> > -   else
> > -      interpolation = INTERP_QUALIFIER_NONE;
> > -
> > -   if (interpolation != INTERP_QUALIFIER_NONE) {
> > -      if (mode != ir_var_shader_in && mode != ir_var_shader_out) {
> > -         _mesa_glsl_error(loc, state,
> > -                          "interpolation qualifier `%s' can only
> > be
> > applied to "
> > -                          "shader inputs or outputs.",
> > -                          interpolation_string(interpolation));
> > -
> > -      }
> > -
> > -      if ((state->stage == MESA_SHADER_VERTEX && mode ==
> > ir_var_shader_in) ||
> > -          (state->stage == MESA_SHADER_FRAGMENT && mode ==
> > ir_var_shader_out)) {
> > -         _mesa_glsl_error(loc, state,
> > -                          "interpolation qualifier `%s' cannot be
> > applied to "
> > -                          "vertex shader inputs or fragment shader
> > outputs",
> > -                          interpolation_string(interpolation));
> > -      }
> > -   } 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))) {
> > +   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;
> > +
> > +   validate_interpolation_qualifier(state, loc,
> > +                                    interpolation,
> > +                                    qual, var_type, mode);
> >  
> >     return interpolation;
> >  }
> > @@ -3575,7 +3717,8 @@ apply_type_qualifier_to_variable(const struct
> > ast_type_qualifier *qual,
> >     }
> >  
> >     var->data.interpolation =
> > -      interpret_interpolation_qualifier(qual, (ir_variable_mode)
> > var->data.mode,
> > +      interpret_interpolation_qualifier(qual, var->type,
> > +                                        (ir_variable_mode) var-
> > > 
> > > data.mode,
> >                                          state, loc);
> >  
> >     /* Does the declaration use the deprecated 'attribute' or
> > 'varying'
> > @@ -4745,124 +4888,6 @@ ast_declarator_list::hir(exec_list
> > *instructions,
> >           var->data.how_declared = ir_var_hidden;
> >        }
> >  
> > -      /* Integer fragment inputs must be qualified with
> > 'flat'.  In
> > GLSL ES,
> > -       * so must integer vertex outputs.
> > -       *
> > -       * From section 4.3.4 ("Inputs") of the GLSL 1.50 spec:
> > -       *    "Fragment shader inputs that are signed or unsigned
> > integers or
> > -       *    integer vectors must be qualified with the
> > interpolation
> > qualifier
> > -       *    flat."
> > -       *
> > -       * From section 4.3.4 ("Input Variables") of the GLSL 3.00
> > ES
> > spec:
> > -       *    "Fragment shader inputs that are, or contain, signed
> > or
> > unsigned
> > -       *    integers or integer vectors must be qualified with the
> > -       *    interpolation qualifier flat."
> > -       *
> > -       * From section 4.3.6 ("Output Variables") of the GLSL 3.00
> > ES
> > spec:
> > -       *    "Vertex shader outputs that are, or contain, signed or
> > unsigned
> > -       *    integers or integer vectors must be qualified with the
> > -       *    interpolation qualifier flat."
> > -       *
> > -       * Note that prior to GLSL 1.50, this requirement applied to
> > vertex
> > -       * outputs rather than fragment inputs.  That creates
> > problems
> > in the
> > -       * presence of geometry shaders, so we adopt the GLSL 1.50
> > rule for all
> > -       * desktop GL shaders.  For GLSL ES shaders, we follow the
> > spec and
> > -       * apply the restriction to both vertex outputs and fragment
> > inputs.
> > -       *
> > -       * Note also that the desktop GLSL specs are missing the
> > text
> > "or
> > -       * contain"; this is presumably an oversight, since there is
> > no
> > -       * reasonable way to interpolate a fragment shader input
> > that
> > contains
> > -       * an integer.
> > -       */
> > -      if (state->is_version(130, 300) &&
> > -          var->type->contains_integer() &&
> > -          var->data.interpolation != INTERP_QUALIFIER_FLAT &&
> > -          ((state->stage == MESA_SHADER_FRAGMENT && var->data.mode
> > == ir_var_shader_in)
> > -           || (state->stage == MESA_SHADER_VERTEX && var-
> > >data.mode
> > == ir_var_shader_out
> > -               && state->es_shader))) {
> > -         const char *var_type = (state->stage ==
> > MESA_SHADER_VERTEX)
> > ?
> > -            "vertex output" : "fragment input";
> > -         _mesa_glsl_error(&loc, state, "if a %s is (or contains) "
> > -                          "an integer, then it must be qualified
> > with 'flat'",
> > -                          var_type);
> > -      }
> > -
> > -      /* Double fragment inputs must be qualified with 'flat'. */
> > -      if (var->type->contains_double() &&
> > -          var->data.interpolation != INTERP_QUALIFIER_FLAT &&
> > -          state->stage == MESA_SHADER_FRAGMENT &&
> > -          var->data.mode == ir_var_shader_in) {
> > -         _mesa_glsl_error(&loc, state, "if a fragment input is (or
> > contains) "
> > -                          "a double, then it must be qualified
> > with
> > 'flat'",
> > -                          var_type);
> > -      }
> > -
> > -      /* Interpolation qualifiers cannot be applied to 'centroid'
> > and
> > -       * 'centroid varying'.
> > -       *
> > -       * From page 29 (page 35 of the PDF) of the GLSL 1.30 spec:
> > -       *    "interpolation qualifiers may only precede the
> > qualifiers in,
> > -       *    centroid in, out, or centroid out in a declaration.
> > They
> > do not apply
> > -       *    to the deprecated storage qualifiers varying or
> > centroid
> > varying."
> > -       *
> > -       * These deprecated storage qualifiers do not exist in GLSL
> > ES
> > 3.00.
> > -       */
> > -      if (state->is_version(130, 0)
> > -          && this->type->qualifier.has_interpolation()
> > -          && this->type->qualifier.flags.q.varying) {
> > -
> > -         const char *i = interpolation_string(var-
> > > 
> > > data.interpolation);
> > -         const char *s;
> > -         if (this->type->qualifier.flags.q.centroid)
> > -            s = "centroid varying";
> > -         else
> > -            s = "varying";
> > -
> > -         _mesa_glsl_error(&loc, state,
> > -                          "qualifier '%s' cannot be applied to the
> > "
> > -                          "deprecated storage qualifier '%s'", i,
> > s);
> > -      }
> > -
> > -
> > -      /* Interpolation qualifiers can only apply to vertex shader
> > outputs and
> > -       * fragment shader inputs.
> > -       *
> > -       * From page 29 (page 35 of the PDF) of the GLSL 1.30 spec:
> > -       *    "Outputs from a vertex shader (out) and inputs to a
> > fragment
> > -       *    shader (in) can be further qualified with one or more
> > of
> > these
> > -       *    interpolation qualifiers"
> > -       *
> > -       * From page 31 (page 37 of the PDF) of the GLSL ES 3.00
> > spec:
> > -       *    "These interpolation qualifiers may only precede the
> > qualifiers
> > -       *    in, centroid in, out, or centroid out in a
> > declaration.
> > They do
> > -       *    not apply to inputs into a vertex shader or outputs
> > from
> > a
> > -       *    fragment shader."
> > -       */
> > -      if (state->is_version(130, 300)
> > -          && this->type->qualifier.has_interpolation()) {
> > -
> > -         const char *i = interpolation_string(var-
> > > 
> > > data.interpolation);
> > -         switch (state->stage) {
> > -         case MESA_SHADER_VERTEX:
> > -            if (this->type->qualifier.flags.q.in) {
> > -               _mesa_glsl_error(&loc, state,
> > -                                "qualifier '%s' cannot be applied
> > to
> > vertex "
> > -                                "shader inputs", i);
> > -            }
> > -            break;
> > -         case MESA_SHADER_FRAGMENT:
> > -            if (this->type->qualifier.flags.q.out) {
> > -               _mesa_glsl_error(&loc, state,
> > -                                "qualifier '%s' cannot be applied
> > to
> > fragment "
> > -                                "shader outputs", i);
> > -            }
> > -            break;
> > -         default:
> > -            break;
> > -         }
> > -      }
> > -
> > -
> >        /* From section 4.3.4 of the GLSL 4.00 spec:
> >         *    "Input variables may not be declared using the patch
> > in
> > qualifier
> >         *    in tessellation control or geometry shaders."
> > @@ -6597,7 +6622,8 @@
> > ast_process_struct_or_iface_block_members(exec_list *instructions,
> >           fields[i].type = field_type;
> >           fields[i].name = decl->identifier;
> >           fields[i].interpolation =
> > -            interpret_interpolation_qualifier(qual, var_mode,
> > state,
> > &loc);
> > +            interpret_interpolation_qualifier(qual, field_type,
> > +                                              var_mode, state,
> > &loc);
> >           fields[i].centroid = qual->flags.q.centroid ? 1 : 0;
> >           fields[i].sample = qual->flags.q.sample ? 1 : 0;
> >           fields[i].patch = qual->flags.q.patch ? 1 : 0;
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-dev mailing list