<div dir="ltr">On 3 March 2013 00:44, Aras Pranckevicius <span dir="ltr"><<a href="mailto:aras@unity3d.com" target="_blank">aras@unity3d.com</a>></span> wrote:<br><div class="gmail_extra"><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

<div><div>>> opt_constant_variable was marking a variable as constant as long as there was<br>
>> exactly one constant assignment to it, but did not take into account that this<br>
>> assignment might be in a dynamic branch or a loop.<br>
>> Was happening on a fragment shader like this:<br>
>><br>
>> uniform float mode;<br>
>> float func (float c) {<br>
>>     if (mode == 2.0)<br>
>>         return c; // was returning 0.1 after optimization!<br>
>>     if (mode == 3.0)<br>
>>     discard;<br>
>>     if (mode == 10.0)<br>
>>         c = 0.1;<br>
>>     return c;<br>
>> }<br>
>> void main() {<br>
>>     vec4 c = gl_FragCoord;<br>
>>     c.x = func(c.x);<br>
>>     gl_FragColor = c;<br>
>> }<br>
>><br>
>> Now, looking further this optimization pass should also not mark variables as<br>
>> const if there was a dereference of them before that first assignment. I had<br>
>> code to do this (a hashtable that would track dereferences before assignment<br>
>> is done). But couldn't come up with a test case that would break the whole set<br>
>> of optimizations that Mesa does (lower jumps, or inlining, ... were getting in<br>
>> the way and hide the bug).<br>
><br>
> I'm not sure I agree with this.  The real problem with the example code you showed<br>
> above is that there's an implicit write to the variable c at the top of the function,<br>
> so c is not in fact constant--it's written twice.  What we should really do is modify<br>
> the optimization pass so that it's aware of the implicit write that happens in "in"<br>
> and "inout" function args.<br>
<br>
</div></div>Attached version two of the patch which does what you suggest - any<br>
ir_var_in, ir_var_const_in or ir_var_inout function args are being<br>
marked as assigned to. Fixes the issue just as well as my initial<br>
patch on several shaders that were problematic before.<br></blockquote><div><br></div><div>Ok, I've taken a deeper look now, and I still have some concerns about this patch:<br><br></div><div>- The patch doesn't compile cleanly on master.  In particular, it looks like it was made using a version of the code prior to commit 42a29d8 (glsl: Eliminate ambiguity between function ins/outs and shader ins/outs).<br>
</div><div><br></div><div>- It seems kludgy to add a visitor for 
ir_function_signature that loops through all the parameters, since the 
default hierarchial visitor for ir_function_signature already does 
that.  Why not just modify the ir_variable visitor so that it increments
 entry->assignment_count when it visits a variable whose mode is ir_var_function_in or ir_var_function_inout?  (Note that the ability to distinguish between function "in" variables and shader "in" variables was added in commit 42a29d8, the commit I mentined above).<br>
<br>- This optimization pass runs in different ways depending on whether it's being run on an unlinked shader or a linked shader.  When it's run on an unlinked shader, it runs via do_constant_variable_unlinked(), which finds each function and visits the function body individually.  As such, it never visits the parameter declarations (or the ir_function_signature node), so the assignment_entry structures it creates for the variable "c" (in your example above) has our_scope=false, and no optimization is performed.  So the bug doesn't manifest itself, and even if it did, your patch would have no effect, since the ir_function_signature node isn't visited.  When the optimization pass is run on a linked shader, function inlining has already been performed, so there is only one ir_function_signature node left (the one for main, which takes no parameters).  So again, the bug doesn't manifest itself, and even if it did, your patch would have no effect.<br>
<br></div><div>Although I agree that opt_constant_variable has problems and could be improved, it seems like I must be missing some crucial piece of information here, since I can't reproduce the bug and I can't understand why your patch would have any effect.  I've made a shader_runner test to try to demonstrate the problem (attached), and it works fine on mesa master as well as the 9.1, 9.0, 8.0, and 7.11 branches.  Can you help me understand what I'm missing?<br>
</div><br></div><div>Finally, in the future would you mind posting patches to the mailing list as inline text rather than attachments?  ("git send-email" is a convenient way to do this.)  It makes them far easier to review since we can comment on the code by simply hitting Reply-all and making review comments alongside the code.  Thanks!<br>
</div></div></div>