Re: [Mesa-dev] [PATCH] GLSL: fix too eager constant variable optimization
On 3 March 2013 00:44, Aras Pranckevicius a...@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!
fp-bogus-constant.shader_test
Description: Binary data
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] GLSL: fix too eager constant variable optimization
- 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). Whoops, indeed. I made in my own modified Mesa fork (GLSL Optimizer, https://github.com/aras-p/glsl-optimizer) that's not up to date with latest master. - 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). Good point, that should be a better approach. 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? Yeah perhaps it can't ever manifest in Mesa's context. In my fork, I'm using Mesa's GLSL compiler optimization passes to do a GLSL-to-GLSL compiler (sounds weird? it kind of is, but works around many mobile platform drivers being really, really weak at GLSL optimization). I do optimization passes very similarly to Mesa, but slightly different at places. But since it's not clear whether my improvements bring any benefit in the context of Mesa, then maybe my patch should just be ignored. And sorry for the wasted time then. 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.) Will do. -- Aras Pranckevičius work: http://unity3d.com home: http://aras-p.info ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] GLSL: fix too eager constant variable optimization
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.
--
Aras Pranckevičius
work: http://unity3d.com
home: http://aras-p.info
glsl-constant-variable-fix-v2.diff
Description: Binary data
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] GLSL: fix too eager constant variable optimization
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. If we resolve the implicit write issue, there should be no harm in marking variables as const if there's a dereference of them before the first assignment, because a dereference of a variable before the first assignment is undefined. Right. I'm not sure what's the correct way of making constant variable pass robust, all I know is that in it's current form it is clearly wrong. If function arguments do get marked as one write, that effectively means they will be excluded from actual optimization until the function is inlined? Maybe that's okay, no sure. -- Aras Pranckevičius work: http://unity3d.com home: http://aras-p.info ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] GLSL: fix too eager constant variable optimization
Hi,
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).
--
Aras Pranckevičius
work: http://unity3d.com
home: http://aras-p.info
glsl-constant-variable-fix.diff
Description: Binary data
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] GLSL: fix too eager constant variable optimization
On 03/01/2013 02:02 AM, Aras Pranckevicius wrote:
Hi,
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).
Good catch! There are a couple white-space issues in the patch, but I
can fix those when I push it. I'll wait a bit for other people to give
review comments. I'll probably also copy the above text into the commit
message.
--
Aras Pranckevičius
work: http://unity3d.com
home: http://aras-p.info
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] GLSL: fix too eager constant variable optimization
On 1 March 2013 02:02, Aras Pranckevicius a...@unity3d.com wrote:
Hi,
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.
If we resolve the implicit write issue, there should be no harm in marking
variables as const if there's a dereference of them before the first
assignment, because a dereference of a variable before the first assignment
is undefined.
I'm also not certain I like the idea of disabling the optimization in cases
where the assignment is in a dynamic branch or a loop. It misses out, for
instance, on the opportunity to optimize this code:
uniform int u;
void main()
{
float c;
for (int i = 0; i u; i++) {
c = 0.5;
...
}
/* It should be fine to constant-fold c in the code below, since
* it is guaranteed to be == 0.5 or undefined, depending on
* the value of u.
*/
gl_FragColor = vec4(c);
}
--
Aras Pranckevičius
work: http://unity3d.com
home: http://aras-p.info
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
