Hello, Please find my comments below:
On Sat, Oct 6, 2018 at 1:07 AM Ian Romanick <i...@freedesktop.org> wrote: > On 10/05/2018 03:02 PM, Ian Romanick wrote: > > On 10/04/2018 07:08 AM, asimiklit.w...@gmail.com wrote: > >> From: Andrii Simiklit <andrii.simik...@globallogic.com> > >> > >> GLSL 3.7 (Identifiers): > >> "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' > >> and leads to regression in clip-distance-out-values.shader_test > >> but probably a fix should be made in the test. > > > > I believe clip-distance-out-values.shader_test is incorrect. The > redeclaration of gl_ClipDistance should have "out". glslang rejects it > with: > > > > ERROR: 0:5: 'redeclaration' : cannot change qualification of > gl_ClipDistance > > ERROR: 1 compilation errors. No code generated. > > > > I think Mark and Clayton would prefer it if the fix to the test landed > first. That way the test never shows up as "fail" to them. > > > > I'll send the fix for this with the new test cases that I mention below. > > > >> 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) > >> > >> Fixes: 8e6cb9fe51a2 "glsl: Refactor AST-to-HIR code handling variable > >> redeclarations" > > > > While this is technically correct, I don't think we want to add a new > compile error to the stable branches. On the outside chance this breaks > some application, I'd rather have it sit on master for a bit first. > > > >> Signed-off-by: Andrii Simiklit <andrii.simik...@globallogic.com> > >> --- > >> src/compiler/glsl/ast_to_hir.cpp | 13 +++++++++---- > >> 1 file changed, 9 insertions(+), 4 deletions(-) > >> > >> diff --git a/src/compiler/glsl/ast_to_hir.cpp > b/src/compiler/glsl/ast_to_hir.cpp > >> index 93e7c8ec33..e26ae6b92a 100644 > >> --- a/src/compiler/glsl/ast_to_hir.cpp > >> +++ b/src/compiler/glsl/ast_to_hir.cpp > >> @@ -4240,10 +4240,15 @@ 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. > >> - */ > >> + > >> + if ((strcmp("gl_ClipDistance", var->name) == 0) && > > > > Hmm... this fixes the specific case mentioned in the old e-mail thread, > but this could occur with any variable. For example, I don't think this > shader should compile, but it does: > > > > #version 110 > > varying float x[]; > > uniform float x[3]; > > uniform int i; > > > > void main() > > { > > gl_Position = vec4(x[i]); > > } > > > > Much to my surprise, glslang does not reject this shader. It looks like > glslang rejects anything that tries to change the storage qualifier on any > built-in variable, but it seems to allow such changes on user-defined > variables. I think that's a bug in glslang. > > > > Let's do this for now. > > > > 1. Change Mesa to reject any storage qualifier change on a built-in > variable. This will match glslang. We can do this by bringing the code by > the comment "Allow verbatim redeclarations of built-in variables. Not > explicitly valid, but some applications do it." up before the big > if-then-else tree. Around line 4235 add something like: > > > > if (earlier->data.how_declared == ir_var_declared_implicitly) { > > /* 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 cannot change qualification of > `%s'", > > var->name); > > Oops... > > } else if (earlier->type != var->type) { > _mesa_glsl_error(&loc, state, > "redeclaration of `%s' has incorrect type", > var->name); > > > } > > } > I have tried to test this version on Intel CI. https://gitlab.freedesktop.org/asimiklit/mesa/commit/9d4f6a97568e7e2720e43601697eaf90908b7aab but it caused many errors: https://mesa-ci.01.org/global_logic/builds/16/group/63a9f0ea7bb98050796b649e85481845 so I modified it in the following way: - } else if (earlier->type != var->type) { + } else if (earlier->type->without_array() != var->type->without_array()) { https://gitlab.freedesktop.org/asimiklit/mesa/commit/7f313c10640a72e37069f54baadaae1bc3d5a0a3 and it helped a bit: https://mesa-ci.01.org/global_logic/builds/17/group/63a9f0ea7bb98050796b649e85481845 I have been investigating the remaining errors. Some part of the errors refer to 'error: redeclaration of `gl_LastFragData' with incorrect qualifiers' (ex. https://mesa-ci.01.org/global_logic/builds/17/results/23218604 ) so probably we have to do some workaround for this variable because it is added as 'ir_var_shader_out' but then it is expected as 'ir_var_auto' in 'get_variable_being_redeclared' function. I think about something like: + const bool qualifiersChangeIsRequired = + (state->has_framebuffer_fetch() && + !state->is_version(130, 300) && + var->type->is_array() && + strcmp(var->name, "gl_LastFragData") == 0); if (earlier->data.how_declared == ir_var_declared_implicitly) { /* 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)) { + var->data.mode == ir_var_shader_in) && + !qualifiersChangeIsRequired) { _mesa_glsl_error(&loc, state, "redeclaration of `%s' with incorrect qualifiers", var->name); What do you think about this workaround? > > > I changed the error message to be more similar to glslang. I like that > wording, but I'm flexible. > > > > We'll also want to remove the other tests for this (which will be > redundant after the above change). > > > > 2. Add a bunch of piglit tests to make sure we match glslang. I've got > a start on this, and I'll CC you on the patches when I send them. > > > > 3. I'll submit a GLSL spec bug to make sure cases like my example above > are intended to be illegal. > > > > 4. Depending on the outcome of #3, update Mesa to match. > > > >> + earlier->data.mode != var->data.mode) { > >> + _mesa_glsl_error(&loc, state, > >> + "redeclaration of '%s %s' with incorrect > qualifiers '%s'.", > >> + mode_string(earlier), > >> + var->name, > >> + mode_string(var)); > >> + } > >> > >> const int size = var->type->array_size(); > >> check_builtin_array_max_size(var->name, size, loc, state); > > _______________________________________________ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > Regards, Andrii.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
