<div dir="ltr"><div>Hello,</div><div> </div><div>Thanks a lot for review.</div><div> </div><div>Regards,</div><div>Andrii. </div> <div class="gmail_quote"><div dir="ltr">On Sat, Nov 10, 2018 at 5:38 AM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com">tarceri@itsqueeze.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Nice! Series is: Reviewed-by: Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a>> On 10/10/18 9:07 am, Ian Romanick wrote: > From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>> > > 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: > >Â Â Â <a href="https://patchwork.freedesktop.org/patch/255201/" rel="noreferrer" target="_blank">https://patchwork.freedesktop.org/patch/255201/</a> > > As far as I understood following mailing thread: > <a href="https://lists.freedesktop.org/archives/piglit/2013-October/007935.html" rel="noreferrer" target="_blank">https://lists.freedesktop.org/archives/piglit/2013-October/007935.html</a> > 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 <<a href="mailto:andrii.simiklit@globallogic.com" target="_blank">andrii.simiklit@globallogic.com</a>> > Signed-off-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>> > --- >Â Â 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 <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div></div>