Hello, Thanks a lot for review.
Regards, Andrii. On Sat, Nov 10, 2018 at 5:38 AM Timothy Arceri <tarc...@itsqueeze.com> wrote: > Nice! Series is: > > Reviewed-by: Timothy Arceri <tarc...@itsqueeze.com> > > On 10/10/18 9:07 am, Ian Romanick wrote: > > From: Ian Romanick <ian.d.roman...@intel.com> > > > > Section 3.7 (Identifiers) of the GLSL spec says: > > > > However, as noted in the specification, there are some cases where > > previously declared variables can be redeclared to change or add > > some property, and predeclared "gl_" names are allowed to be > > redeclared in a shader only for these specific purposes. More > > generally, it is an error to redeclare a variable, including those > > starting "gl_". > > > > This patch should fix piglit tests: > > clip-distance-redeclare-without-inout.frag > > clip-distance-redeclare-without-inout.vert > > > > However, this causes a regression in > > clip-distance-out-values.shader_test. A fix for that test has been sent > > to the piglit list for review: > > > > https://patchwork.freedesktop.org/patch/255201/ > > > > As far as I understood following mailing thread: > > https://lists.freedesktop.org/archives/piglit/2013-October/007935.html > > looks like we have accepted to remove an ability to change qualifiers > > but have not done it yet. Unless I missed something) > > > > v2 (idr): Move 'earlier->data.mode != var->data.mode' test much earlier > > in the function. Add special handling for gl_LastFragData. > > > > Signed-off-by: Andrii Simiklit <andrii.simik...@globallogic.com> > > Signed-off-by: Ian Romanick <ian.d.roman...@intel.com> > > --- > > src/compiler/glsl/ast_to_hir.cpp | 51 > +++++++++++++++++++++------------------- > > 1 file changed, 27 insertions(+), 24 deletions(-) > > > > diff --git a/src/compiler/glsl/ast_to_hir.cpp > b/src/compiler/glsl/ast_to_hir.cpp > > index 1082d6c91cf..2e4c9ef6776 100644 > > --- a/src/compiler/glsl/ast_to_hir.cpp > > +++ b/src/compiler/glsl/ast_to_hir.cpp > > @@ -4238,6 +4238,29 @@ get_variable_being_redeclared(ir_variable > **var_ptr, YYLTYPE loc, > > > > *is_redeclaration = true; > > > > + if (earlier->data.how_declared == ir_var_declared_implicitly) { > > + /* Verify that the redeclaration of a built-in does not change the > > + * storage qualifier. There are a couple special cases. > > + * > > + * 1. Some built-in variables that are defined as 'in' in the > > + * specification are implemented as system values. Allow > > + * ir_var_system_value -> ir_var_shader_in. > > + * > > + * 2. gl_LastFragData is implemented as a ir_var_shader_out, but > the > > + * specification requires that redeclarations omit any > qualifier. > > + * Allow ir_var_shader_out -> ir_var_auto for this one > variable. > > + */ > > + if (earlier->data.mode != var->data.mode && > > + !(earlier->data.mode == ir_var_system_value && > > + var->data.mode == ir_var_shader_in) && > > + !(strcmp(var->name, "gl_LastFragData") == 0 && > > + var->data.mode == ir_var_auto)) { > > + _mesa_glsl_error(&loc, state, > > + "redeclaration cannot change qualification of > `%s'", > > + var->name); > > + } > > + } > > + > > /* From page 24 (page 30 of the PDF) of the GLSL 1.50 spec, > > * > > * "It is legal to declare an array without a size and then > > @@ -4246,11 +4269,6 @@ get_variable_being_redeclared(ir_variable > **var_ptr, YYLTYPE loc, > > */ > > if (earlier->type->is_unsized_array() && var->type->is_array() > > && (var->type->fields.array == earlier->type->fields.array)) { > > - /* FINISHME: This doesn't match the qualifiers on the two > > - * FINISHME: declarations. It's not 100% clear whether this is > > - * FINISHME: required or not. > > - */ > > - > > const int size = var->type->array_size(); > > check_builtin_array_max_size(var->name, size, loc, state); > > if ((size > 0) && (size <= earlier->data.max_array_access)) { > > @@ -4342,28 +4360,13 @@ get_variable_being_redeclared(ir_variable > **var_ptr, YYLTYPE loc, > > earlier->data.precision = var->data.precision; > > earlier->data.memory_coherent = var->data.memory_coherent; > > > > - } else if (earlier->data.how_declared == ir_var_declared_implicitly > && > > - state->allow_builtin_variable_redeclaration) { > > + } else if ((earlier->data.how_declared == ir_var_declared_implicitly > && > > + state->allow_builtin_variable_redeclaration) || > > + allow_all_redeclarations) { > > /* Allow verbatim redeclarations of built-in variables. Not > explicitly > > * valid, but some applications do it. > > */ > > - if (earlier->data.mode != var->data.mode && > > - !(earlier->data.mode == ir_var_system_value && > > - var->data.mode == ir_var_shader_in)) { > > - _mesa_glsl_error(&loc, state, > > - "redeclaration of `%s' with incorrect > qualifiers", > > - var->name); > > - } else if (earlier->type != var->type) { > > - _mesa_glsl_error(&loc, state, > > - "redeclaration of `%s' has incorrect type", > > - var->name); > > - } > > - } else if (allow_all_redeclarations) { > > - if (earlier->data.mode != var->data.mode) { > > - _mesa_glsl_error(&loc, state, > > - "redeclaration of `%s' with incorrect > qualifiers", > > - var->name); > > - } else if (earlier->type != var->type) { > > + if (earlier->type != var->type) { > > _mesa_glsl_error(&loc, state, > > "redeclaration of `%s' has incorrect type", > > var->name); > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
