[Mesa-stable] [Mesa-dev] [PATCH 1/3] i965/fs: Properly handle sign(-abs(x))

[Ian Romanick]

On 07/05/2018 06:22 PM, Caio Marcelo de Oliveira Filho wrote:
> Reviewed-by: Caio Marcelo de Oliveira Filho <caio.oliveira at intel.com>
> 
> Tested the patch with the shaders mentioned.
> 
> Do you think would be worth to have a pass at NIR level to perform
> these transformations?

Possibly.  In many cases, it is better to keep a negation or an abs on
the inside of an expression instead of the outside (e.g., sign(-x) vs
-sign(x)).  We can almost always propagate an outer negation to the
user, but there are some cases, such as when the user is in a different
basic block, where it cannot be done.  In those cases, the outer unary
op results in an extra move.  It is difficult to move unary ops inside
because it can interfere with other optimizations.  When I have tried
this in the past, it has generally been an even split between helps / hurts.

> Thanks,
> Caio
> 
> 
> On Wed, Jun 27, 2018 at 07:37:57AM -0700, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> Fixes new piglit tests:
>>
>>  - glsl-1.10/execution/fs-sign-neg-abs.shader_test
>>  - glsl-1.10/execution/fs-sign-sat-neg-abs.shader_test
>>  - glsl-1.10/execution/vs-sign-neg-abs.shader_test
>>  - glsl-1.10/execution/vs-sign-sat-neg-abs.shader_test
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> Cc: mesa-stable at lists.freedesktop.org
>> ---
>>  src/intel/compiler/brw_fs_nir.cpp | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/intel/compiler/brw_fs_nir.cpp b/src/intel/compiler/brw_fs_nir.cpp
>> index 0abb4798e70..2ad7a2d0dfc 100644
>> --- a/src/intel/compiler/brw_fs_nir.cpp
>> +++ b/src/intel/compiler/brw_fs_nir.cpp
>> @@ -807,11 +807,20 @@ fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)
>>  
>>     case nir_op_fsign: {
>>        if (op[0].abs) {
>> -         /* Straightforward since the source can be assumed to be
>> -          * non-negative.
>> +         /* Straightforward since the source can be assumed to be either
>> +          * strictly >= 0 or strictly <= 0 depending on the setting of the
>> +          * negate flag.
>>            */
>>           set_condmod(BRW_CONDITIONAL_NZ, bld.MOV(result, op[0]));
>> -         set_predicate(BRW_PREDICATE_NORMAL, bld.MOV(result, brw_imm_f(1.0f)));
>> +
>> +         inst = (op[0].negate)
>> +            ? bld.MOV(result, brw_imm_f(-1.0f))
>> +            : bld.MOV(result, brw_imm_f(1.0f));
>> +
>> +         set_predicate(BRW_PREDICATE_NORMAL, inst);
>> +
>> +         if (instr->dest.saturate)
>> +            inst->saturate = true;
>>  
>>        } else if (type_sz(op[0].type) < 8) {
>>           /* AND(val, 0x80000000) gives the sign bit.
>> -- 
>> 2.14.4
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>