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 <[email protected]> > > > --- > > > 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 > > [email protected] > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev -- Br, Andres
signature.asc
Description: This is a digitally signed message part
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
