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

[Kenneth Graunke]

On 03/01/2014 09:33 AM, Ian Romanick wrote:
> 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?

Normally, yes, except with the caveat that these are expression trees,
so who knows what the computation is or if anything would find it.

Notably, the shader-db stats didn't find any instances where we got more
instructions (even in fs8), so at least i965 wasn't finding any MAD
opportunities.

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

Yeah, I shied away from that because opt_algebraic has never done that.

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140301/e9f5d835/attachment-0001.pgp>