[Mesa-dev] [PATCH] glsl: Checks for interpolation into its own function.
Andres Gomez
agomez at igalia.com
Thu Apr 28 14:56:18 UTC 2016
On Tue, 2016-04-26 at 12:16 +0200, Samuel Iglesias Gonsálvez wrote:
> On 04/04/16 18:50, 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
> > tml
> > and Khronos bug #15671.
> >
> Do we have news about this Khronos bug? Are the piglit tests pushed
> upstream?
About Khronos' bug, no idea since I don't have access to the bugzilla.
The piglit tests are landing shortly since they have been already
reviewed (I hope in a matter of hours).
> Assuming no piglit/dEQP regressions, this patch is:
I run piglit/dEQP before sending for review and didn't observe any
regressions.
> Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
Thanks!
>
> Sam
>
> >
> > 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;
> >
--
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/20160428/249e91ff/attachment-0001.sig>
More information about the mesa-dev
mailing list