<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><div class="gmail_quote"><div>Hello,</div><div><br></div><div>Please find my comments below:</div><div><br></div><div dir="ltr">On Sat, Oct 6, 2018 at 1:07 AM Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 10/05/2018 03:02 PM, Ian Romanick wrote:<br>
> On 10/04/2018 07:08 AM, <a href="mailto:asimiklit.work@gmail.com" target="_blank">asimiklit.work@gmail.com</a> wrote:<br>
>> From: Andrii Simiklit <<a href="mailto:andrii.simiklit@globallogic.com" target="_blank">andrii.simiklit@globallogic.com</a>><br>
>><br>
>> GLSL 3.7 (Identifiers):<br>
>> "However, as noted in the specification, there are some cases where<br>
>> previously declared variables can be redeclared to change or add some<br>
>> property, and predeclared "gl_" names are allowed to be redeclared in a<br>
>> shader only for these specific purposes. More generally, it is an error to<br>
>> redeclare a variable, including those starting "gl_"."<br>
>><br>
>> This patch should fix piglit tests:<br>
>> 'clip-distance-redeclare-without-inout.frag'<br>
>> 'clip-distance-redeclare-without-inout.vert'<br>
>> and leads to regression in clip-distance-out-values.shader_test<br>
>> but probably a fix should be made in the test.<br>
> <br>
> I believe clip-distance-out-values.shader_test is incorrect. The redeclaration of gl_ClipDistance should have "out". glslang rejects it with:<br>
> <br>
> ERROR: 0:5: 'redeclaration' : cannot change qualification of gl_ClipDistance<br>
> ERROR: 1 compilation errors. No code generated.<br>
> <br>
> 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.<br>
> <br>
> I'll send the fix for this with the new test cases that I mention below.<br>
> <br>
>> As far as I understood following mailing thread:<br>
>> <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><br>
>> looks like we have accepted to remove an ability to change qualifiers<br>
>> but have not done it yet. Unless I missed something)<br>
>><br>
>> Fixes: 8e6cb9fe51a2 "glsl: Refactor AST-to-HIR code handling variable<br>
>> redeclarations"<br>
> <br>
> 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.<br>
> <br>
>> Signed-off-by: Andrii Simiklit <<a href="mailto:andrii.simiklit@globallogic.com" target="_blank">andrii.simiklit@globallogic.com</a>><br>
>> ---<br>
>> src/compiler/glsl/ast_to_hir.cpp | 13 +++++++++----<br>
>> 1 file changed, 9 insertions(+), 4 deletions(-)<br>
>><br>
>> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp<br>
>> index 93e7c8ec33..e26ae6b92a 100644<br>
>> --- a/src/compiler/glsl/ast_to_hir.cpp<br>
>> +++ b/src/compiler/glsl/ast_to_hir.cpp<br>
>> @@ -4240,10 +4240,15 @@ get_variable_being_redeclared(ir_variable **var_ptr, YYLTYPE loc,<br>
>> */<br>
>> if (earlier->type->is_unsized_array() && var->type->is_array()<br>
>> && (var->type->fields.array == earlier->type->fields.array)) {<br>
>> - /* FINISHME: This doesn't match the qualifiers on the two<br>
>> - * FINISHME: declarations. It's not 100% clear whether this is<br>
>> - * FINISHME: required or not.<br>
>> - */<br>
>> +<br>
>> + if ((strcmp("gl_ClipDistance", var->name) == 0) &&<br>
> <br>
> 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:<br>
> <br>
> #version 110<br>
> varying float x[];<br>
> uniform float x[3];<br>
> uniform int i;<br>
> <br>
> void main()<br>
> {<br>
> gl_Position = vec4(x[i]);<br>
> }<br>
> <br>
> 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.<br>
> <br>
> Let's do this for now.<br>
> <br>
> 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:<br>
> <br>
> if (earlier->data.how_declared == ir_var_declared_implicitly) {<br>
> /* Allow verbatim redeclarations of built-in variables. Not explicitly<br>
> * valid, but some applications do it.<br>
> */<br>
> if (earlier->data.mode != var->data.mode &&<br>
> !(earlier->data.mode == ir_var_system_value &&<br>
> var->data.mode == ir_var_shader_in)) {<br>
> _mesa_glsl_error(&loc, state,<br>
> "redeclaration cannot change qualification of `%s'",<br>
> var->name);<br>
<br>
Oops...<br>
<br>
} else if (earlier->type != var->type) {<br>
_mesa_glsl_error(&loc, state,<br>
"redeclaration of `%s' has incorrect type",<br>
var->name);<br>
<br>
> }<br>
> }<br></blockquote><br>I have tried to test this version on Intel CI.</div><div class="gmail_quote"><br></div><div class="gmail_quote"><a href="https://gitlab.freedesktop.org/asimiklit/mesa/commit/9d4f6a97568e7e2720e43601697eaf90908b7aab">https://gitlab.freedesktop.org/asimiklit/mesa/commit/9d4f6a97568e7e2720e43601697eaf90908b7aab</a><br><br>but it caused many errors:<br><br><a href="https://mesa-ci.01.org/global_logic/builds/16/group/63a9f0ea7bb98050796b649e85481845">https://mesa-ci.01.org/global_logic/builds/16/group/63a9f0ea7bb98050796b649e85481845</a><br><br>so I modified it in the following way:<br><br>- } else if (earlier->type != var->type) {<br>+ } else if (earlier->type->without_array() != var->type->without_array()) {<br><br><a href="https://gitlab.freedesktop.org/asimiklit/mesa/commit/7f313c10640a72e37069f54baadaae1bc3d5a0a3">https://gitlab.freedesktop.org/asimiklit/mesa/commit/7f313c10640a72e37069f54baadaae1bc3d5a0a3</a></div><div class="gmail_quote"><br>and it helped a bit:<br><br><a href="https://mesa-ci.01.org/global_logic/builds/17/group/63a9f0ea7bb98050796b649e85481845">https://mesa-ci.01.org/global_logic/builds/17/group/63a9f0ea7bb98050796b649e85481845</a></div><div class="gmail_quote"><br>I have been investigating the remaining errors. <br></div>Some part of the errors refer to 'error: redeclaration of `gl_LastFragData' with incorrect qualifiers'</div><div>(ex. <a href="https://mesa-ci.01.org/global_logic/builds/17/results/23218604">https://mesa-ci.01.org/global_logic/builds/17/results/23218604</a> )</div><div>so probably we have to do some workaround for this variable</div><div>because it is added as 'ir_var_shader_out' but then</div><div> it is expected as 'ir_var_auto' in 'get_variable_being_redeclared' function.</div><div><br></div><div>I think about something like:</div><div>+ const bool qualifiersChangeIsRequired =<br>+ (state->has_framebuffer_fetch() &&<br>+ !state->is_version(130, 300) &&<br>+ var->type->is_array() &&<br>+ strcmp(var->name, "gl_LastFragData") == 0);<br> if (earlier->data.how_declared == ir_var_declared_implicitly) {<br> /* Allow verbatim redeclarations of built-in variables. Not explicitly<br> * valid, but some applications do it.<br> */<br> if (earlier->data.mode != var->data.mode &&<br> !(earlier->data.mode == ir_var_system_value &&<br>- var->data.mode == ir_var_shader_in)) {<br>+ var->data.mode == ir_var_shader_in) &&<br>+ !qualifiersChangeIsRequired) {<br> _mesa_glsl_error(&loc, state,<br> "redeclaration of `%s' with incorrect qualifiers",<br> var->name);</div><div><br></div><div>What do you think about this workaround?<br></div><div><div class="gmail_quote"><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> <br>
> I changed the error message to be more similar to glslang. I like that wording, but I'm flexible.<br>
> <br>
> We'll also want to remove the other tests for this (which will be redundant after the above change).<br>
> <br>
> 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.<br>
> <br>
> 3. I'll submit a GLSL spec bug to make sure cases like my example above are intended to be illegal.<br>
> <br>
> 4. Depending on the outcome of #3, update Mesa to match.<br>
> <br>
>> + earlier->data.mode != var->data.mode) {<br>
>> + _mesa_glsl_error(&loc, state,<br>
>> + "redeclaration of '%s %s' with incorrect qualifiers '%s'.",<br>
>> + mode_string(earlier),<br>
>> + var->name,<br>
>> + mode_string(var));<br>
>> + }<br>
>> <br>
>> const int size = var->type->array_size();<br>
>> check_builtin_array_max_size(var->name, size, loc, state);<br>
> _______________________________________________<br>
> mesa-dev mailing list<br>
> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
> <br>
<br></blockquote><div>Regards,</div><div>Andrii.<br></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div></div>