[Mesa-dev] [PATCH 4/8] i965/fs: Fix nir_op_fsign of absolute value.

[Ian Romanick]

On 01/25/2017 01:42 PM, Francisco Jerez wrote:
> Ian Romanick <idr at freedesktop.org> writes:
> 
>> On 01/24/2017 03:26 PM, Francisco Jerez wrote:
>>> This does point at the front-end emitting silly code that could have
>>> been optimized out, but the current fsign implementation would emit
>>> bogus IR if abs was set for the argument (because it would apply the
>>> abs modifier on an unsigned integer type), and we shouldn't rely on
>>> the upper layer's optimization passes for correctness.
>>
>> Other than the atan2 code you emit later in the series, is there a test
>> for this?
>>
> 
> PATCH 5 would cause a pile of atan tests to regress without this, but
> see the attachment for a test-case that reproduces the problem in
> isolation.
> 
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>> index e1ab598..e0c2fa0 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>>> @@ -701,7 +701,14 @@ fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)
>>>        break;
>>>  
>>>     case nir_op_fsign: {
>>> -      if (type_sz(op[0].type) < 8) {
>>> +      if (op[0].abs) {
>>> +         /* Straightforward since the source can be assumed to be
>>> +          * non-negative.
>>> +          */
>>> +         set_condmod(BRW_CONDITIONAL_NZ, bld.MOV(result, op[0]));
>>> +         set_predicate(BRW_PREDICATE_NORMAL, bld.MOV(result, brw_imm_f(1.0f)));
>>
>> Does this work for DF source?
>>
> Yup.
> 
>> If we had an optimization pass for this, it would probably map
>> fsign(abs(a)) to float(a != 0) or double(a != 0).  This is different
>> from what we would generate for that, but I don't know which is better.
>>
> 
> The main reason I did it that way was because it's able to handle double
> or single precision as-is without any special cases -- To do double(a !=
> 0) you need a 2-src CMP instruction which means you cannot use a DF
> immediate to compare against.  float(a != 0) is probably marginally
> better for single-precision though, because the second instruction would
> have a higher chance of getting optimized out -- If you like I'll make
> the change and deal with the restrictions of DF immediates.

Ah... that makes sense.  This patch is

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

I believe this should also get tagged for 13.x and 17.0 stable.

>>> +
>>> +      } else if (type_sz(op[0].type) < 8) {
>>>           /* AND(val, 0x80000000) gives the sign bit.
>>>            *
>>>            * Predicated OR ORs 1.0 (0x3f800000) with the sign bit if val is not


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170126/743781bd/attachment.sig>