<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">2018-02-09 0:16 GMT+01:00 Matt Turner <span dir="ltr"><<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">On Mon, Feb 5, 2018 at 7:16 PM, Vlad Golovkin<br>
<<a href="mailto:vlad.golovkin.mail@gmail.com">vlad.golovkin.mail@gmail.com</a>> wrote:<br>
> val->i32[swizzle[i]] is guaranteed to have non-positive value before the<br>
> __is_power_of_two call, so unary minus is equivalent to abs in this case.<br>
> ---<br>
>  src/compiler/nir/nir_search_<wbr>helpers.h | 2 +-<br>
>  1 file changed, 1 insertion(+), 1 deletion(-)<br>
><br>
> diff --git a/src/compiler/nir/nir_search_<wbr>helpers.h b/src/compiler/nir/nir_search_<wbr>helpers.h<br>
> index 2e3bd137d6..66e1546ae6 100644<br>
> --- a/src/compiler/nir/nir_search_<wbr>helpers.h<br>
> +++ b/src/compiler/nir/nir_search_<wbr>helpers.h<br>
> @@ -80,7 +80,7 @@ is_neg_power_of_two(nir_alu_<wbr>instr *instr, unsigned src, unsigned num_components,<br>
>        case nir_type_int:<br>
>           if (val->i32[swizzle[i]] > 0)<br></span></blockquote><div><br></div><div>Hi,</div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">
>              return false;<br>
> -         if (!__is_power_of_two(abs(val-><wbr>i32[swizzle[i]])))<br>
> +         if (!__is_power_of_two(-val->i32[<wbr>swizzle[i]]))<br>
>              return false;<br>
<br>
</span>I think your transformation is correct, but unnecessary and confusing.<br>
<br>
You're right that val->i32[swizzle[i] must be 0 or negative.<br>
__is_power_of_two() takes an unsigned value, so we need to remove the<br>
sign bit, which can be done with a negation or an abs().<br>
<br>
It takes more effort for me to understand why the negation is correct,<br>
while the abs() is obvious.<br></blockquote><div><br></div><div>abs(x) can be defined as: </div><div>#define abs(x) ((x) > 0 ? (x) : -(x))</div><div><br></div><div>(other than evaluating (x) twice; also, for 2s-complement arithmetic, > and be replaced with >=)</div><div><br></div><div>Since we already know (x) > 0 doesn't hold, abs(x) can be simplified to -(x).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Anyone else have a different opinion?</blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>As such, I would recommend changing it to:</div><div><br></div><div>__is_power_of_two(-(unsigned)val->i32[swizzle[i]])</div><div><br></div><div>Regards,</div><div>Gustaw Smolarczyk</div><div><br></div><div>[1] <a href="http://man7.org/linux/man-pages/man3/abs.3.html">http://man7.org/linux/man-pages/man3/abs.3.html</a></div><div>[2] <a href="https://wandbox.org/permlink/bqWG5gvNz4WUWFna">https://wandbox.org/permlink/bqWG5gvNz4WUWFna</a></div><div>[3] <a href="https://stackoverflow.com/a/8917528">https://stackoverflow.com/a/8917528</a></div></div></div></div>