[Mesa-dev] [PATCH] nir: remove the abs call in is_neg_power_of_two

[Gustaw Smolarczyk]

2018-02-09 0:16 GMT+01:00 Matt Turner <mattst88 at gmail.com>:

> On Mon, Feb 5, 2018 at 7:16 PM, Vlad Golovkin
> <vlad.golovkin.mail at gmail.com> wrote:
> > val->i32[swizzle[i]] is guaranteed to have non-positive value before the
> > __is_power_of_two call, so unary minus is equivalent to abs in this case.
> > ---
> >  src/compiler/nir/nir_search_helpers.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/compiler/nir/nir_search_helpers.h
> b/src/compiler/nir/nir_search_helpers.h
> > index 2e3bd137d6..66e1546ae6 100644
> > --- a/src/compiler/nir/nir_search_helpers.h
> > +++ b/src/compiler/nir/nir_search_helpers.h
> > @@ -80,7 +80,7 @@ is_neg_power_of_two(nir_alu_instr *instr, unsigned
> src, unsigned num_components,
> >        case nir_type_int:
> >           if (val->i32[swizzle[i]] > 0)
>

Hi,
I would change it to >= 0, as when the value is zero it cannot be a power
of two. Either way, -0 == 0, so it doesn't matter from correctness
perspective.


> >              return false;
> > -         if (!__is_power_of_two(abs(val->i32[swizzle[i]])))
> > +         if (!__is_power_of_two(-val->i32[swizzle[i]]))
> >              return false;
>
> I think your transformation is correct, but unnecessary and confusing.
>
> You're right that val->i32[swizzle[i] must be 0 or negative.
> __is_power_of_two() takes an unsigned value, so we need to remove the
> sign bit, which can be done with a negation or an abs().
>
> It takes more effort for me to understand why the negation is correct,
> while the abs() is obvious.
>

abs(x) can be defined as:
#define abs(x) ((x) > 0 ? (x) : -(x))

(other than evaluating (x) twice; also, for 2s-complement arithmetic, > and
be replaced with >=)

Since we already know (x) > 0 doesn't hold, abs(x) can be simplified to
-(x).


>
> Anyone else have a different opinion?


One issue with the previous code is that abs(INT_MIN) is undefined [1], as
you cannot represent -INT_MIN in int since (int)-(unsigned)INT_MIN ==
INT_MIN [2]. Similarly (I think - I lack a quote here other than SO [3])
taking a negative of INT_MIN as-is is undefined, as it cannot be
represented in the int type without wrapping too - you first have to
convert it to unsigned.

However, when you take the negative of an int converted to unsigned, all
will be good. For 2s-complement arithmetic it doesn't matter whether you
take a negative from a signed number as-is or an signed number converted to
unsigned (the only thing that changes is the interpretation of the
resulting bits). And this also holds for INT_MIN, for example for 32-bit
int, INT_MIN == -2^31 and -(unsigned)INT_MIN == (unsigned)INT_MIN == 2^31.
In other words - by first converting to unsigned, we make any overflow
happening during computation defined as wrapping, and we want the result to
be unsigned anyway.

As such, I would recommend changing it to:

__is_power_of_two(-(unsigned)val->i32[swizzle[i]])

Regards,
Gustaw Smolarczyk

[1] http://man7.org/linux/man-pages/man3/abs.3.html
[2] https://wandbox.org/permlink/bqWG5gvNz4WUWFna
[3] https://stackoverflow.com/a/8917528
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180209/ae89f995/attachment.html>