2018-02-09 0:16 GMT+01:00 Matt Turner <matts...@gmail.com>: > On Mon, Feb 5, 2018 at 7:16 PM, Vlad Golovkin > <vlad.golovkin.m...@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
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
