[Mesa-dev] [PATCH] GLSL: fix too eager constant variable optimization

Paul Berry stereotype441 at gmail.com
Wed Mar 6 09:59:44 PST 2013


On 3 March 2013 00:44, Aras Pranckevicius <aras at unity3d.com> wrote:

> >> opt_constant_variable was marking a variable as constant as long as
> there was
> >> exactly one constant assignment to it, but did not take into account
> that this
> >> assignment might be in a dynamic branch or a loop.
> >> Was happening on a fragment shader like this:
> >>
> >> uniform float mode;
> >> float func (float c) {
> >>     if (mode == 2.0)
> >>         return c; // was returning 0.1 after optimization!
> >>     if (mode == 3.0)
> >>     discard;
> >>     if (mode == 10.0)
> >>         c = 0.1;
> >>     return c;
> >> }
> >> void main() {
> >>     vec4 c = gl_FragCoord;
> >>     c.x = func(c.x);
> >>     gl_FragColor = c;
> >> }
> >>
> >> Now, looking further this optimization pass should also not mark
> variables as
> >> const if there was a dereference of them before that first assignment.
> I had
> >> code to do this (a hashtable that would track dereferences before
> assignment
> >> is done). But couldn't come up with a test case that would break the
> whole set
> >> of optimizations that Mesa does (lower jumps, or inlining, ... were
> getting in
> >> the way and hide the bug).
> >
> > I'm not sure I agree with this.  The real problem with the example code
> you showed
> > above is that there's an implicit write to the variable c at the top of
> the function,
> > so c is not in fact constant--it's written twice.  What we should really
> do is modify
> > the optimization pass so that it's aware of the implicit write that
> happens in "in"
> > and "inout" function args.
>
> Attached version two of the patch which does what you suggest - any
> ir_var_in, ir_var_const_in or ir_var_inout function args are being
> marked as assigned to. Fixes the issue just as well as my initial
> patch on several shaders that were problematic before.
>

Ok, I've taken a deeper look now, and I still have some concerns about this
patch:

- 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).

- 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).

- 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.

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?

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!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130306/f1cef1e9/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fp-bogus-constant.shader_test
Type: application/octet-stream
Size: 418 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130306/f1cef1e9/attachment-0001.obj>


More information about the mesa-dev mailing list