[Mesa-dev] [PATCH] glsl: Checks for interpolation into its own function.
Andres Gomez
agomez at igalia.com
Tue Apr 19 08:44:26 UTC 2016
Hi,
On Tue, 2016-04-19 at 08:53 +1000, Timothy Arceri wrote:
> 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?
Sorry, I should have made this more clear and made a cross reference
from the related piglit patch.
As said in this mail, there was a discussion about this at:
https://lists.freedesktop.org/archives/mesa-dev/2016-March/109117.html
And, also, Khronos bug #15671.
In addition, I sent a generator for piglit which creates tests for this
scenario. Several of them won't pass unless this patch is merged in
mesa:
https://lists.freedesktop.org/archives/piglit/2016-April/019340.html
Thanks!
>
> >
> >
> > 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
--
Br,
Andres
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160419/8001a7c9/attachment-0001.sig>
More information about the mesa-dev
mailing list