[Mesa-dev] [PATCH] glsl: Fix broken LRP algebraic optimization.

[Ian Romanick]

On 03/01/2014 12:18 AM, Kenneth Graunke wrote:
> opt_algebraic was translating lrp(x, 0, a) into add(x, -mul(x, a)).
>
> Unfortunately, this references "x" twice, which is invalid in the IR,
> leading to assertion failures in the validator.

Right... and Matt probably didn't notice this because he tests release 
builds because they make piglit runs so much faster.  Hmm...  isn't -Og 
credible yet? :(

> Normally, cloning IR solves this.  However, "x" could actually be an
> arbitrary expression tree, so copying it could result in huge piles
> of wasted computation.  This is why we avoid reusing subexpressions.

The other way to solve this is to assign x to a temporary.  Isn't x-a*x 
slightly better because it can be implemented as MAD?

I don't have a strong opinion... if you don't feel like doing a temp,

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> Instead, transform it into mul(x, add(1.0, -a)), which is equivalent
> but doesn't need two references to "x".
>
> Fixes a regression since d5fa8a95621169, which isn't in any stable
> branches.  Fixes 18 shaders in shader-db (bastion and yofrankie).
>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>   src/glsl/opt_algebraic.cpp | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> Here's the shader-db output comparing master to this:
>
> GAINED: shaders/bastion/48.shader_test fs16
> GAINED: shaders/bastion/48.shader_test fs8
> GAINED: shaders/bastion/48.shader_test vs
> GAINED: shaders/yofrankie/27.shader_test fs16
> GAINED: shaders/yofrankie/27.shader_test fs8
> GAINED: shaders/yofrankie/27.shader_test vs
> GAINED: shaders/yofrankie/30.shader_test fs16
> GAINED: shaders/yofrankie/30.shader_test fs8
> GAINED: shaders/yofrankie/30.shader_test vs
> GAINED: shaders/yofrankie/48.shader_test fs16
> GAINED: shaders/yofrankie/48.shader_test fs8
> GAINED: shaders/yofrankie/48.shader_test vs
> GAINED: shaders/yofrankie/87.shader_test fs16
> GAINED: shaders/yofrankie/87.shader_test fs8
> GAINED: shaders/yofrankie/87.shader_test vs
> GAINED: shaders/yofrankie/9.shader_test fs16
> GAINED: shaders/yofrankie/9.shader_test fs8
> GAINED: shaders/yofrankie/9.shader_test vs
>
> total instructions in shared programs: 1726391 -> 1726391 (0.00%)
> instructions in affected programs:     0 -> 0
> GAINED:                                18
> LOST:
>
> diff --git a/src/glsl/opt_algebraic.cpp b/src/glsl/opt_algebraic.cpp
> index 778638c..5c49a78 100644
> --- a/src/glsl/opt_algebraic.cpp
> +++ b/src/glsl/opt_algebraic.cpp
> @@ -571,7 +571,9 @@ ir_algebraic_visitor::handle_expression(ir_expression *ir)
>         } else if (is_vec_zero(op_const[0])) {
>            return mul(ir->operands[1], ir->operands[2]);
>         } else if (is_vec_zero(op_const[1])) {
> -         return add(ir->operands[0], neg(mul(ir->operands[0], ir->operands[2])));
> +         unsigned op2_components = ir->operands[2]->type->vector_elements;
> +         ir_constant *one = new(mem_ctx) ir_constant(1.0f, op2_components);
> +         return mul(ir->operands[0], add(one, neg(ir->operands[2])));
>         }
>         break;
>
>