[Beignet] [PATCH] fix a powr function issue in cpu compiler math

[Meng, Mengmeng]

Thanks your comments. I'll send out v2 to fix the issues.

-----Original Message-----
From: Matt Turner [mailto:mattst88 at gmail.com] 
Sent: Thursday, July 30, 2015 1:37 AM
To: Meng, Mengmeng
Cc: beignet at lists.freedesktop.org
Subject: Re: [Beignet] [PATCH] fix a powr function issue in cpu compiler math

On Sun, Jul 19, 2015 at 6:33 AM, Meng Mengmeng <mengmeng.meng at intel.com> wrote:
> In OpenCL spec, gentype powr(gentype x, gentype y). In the meantime, 
> added edge tests for powr.
> ---
>  utests/utest_math_gen.py | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/utests/utest_math_gen.py b/utests/utest_math_gen.py index 
> 83edcc3..24ddaa4 100755
> --- a/utests/utest_math_gen.py
> +++ b/utests/utest_math_gen.py
> @@ -467,14 +467,30 @@ static float pown(float x, int y){
>    pownUtests = 
> func('pown','pown',[pown_input_type1,pown_input_type2],pown_output_typ
> e,[pown_input_values1,pown_input_values2],'16 * FLT_ULP', 
> pown_cpu_func)
>
>    ##### gentype powr(gentype x, gentype y)
> -  powr_input_values1 = [80, -80, 3.14, -3.14, 0.5, 1, -1, 
> 0.0,6,1500.24,-1500.24]
> -  powr_input_values2 = [5,6,7,8,10,11,12,13,14,0,12]
> +  powr_input_values1 = [80,-80,3.14,1, 1.257,+0, -0,+0,-0,      +0, -0,  +1, +1,  -80,              0,-0,0,-0, 'INFINITY','INFINITY',+1,+1,0,2.5,'NAN','NAN','NAN' ]
> +  powr_input_values2 = [5.5,6,7, +0,-0,-1,-15.67,'-INFINITY', '-INFINITY',1,  -2.7,10.5, 3.1415,3.5,-0,-0,0,0,   0,       -0,'INFINITY','-INFINITY','NAN','NAN',-1.5,0,1.5]
>    powr_input_type1 = ['float','float2','float4','float8','float16']
>    powr_input_type2 = ['float','float2','float4','float8','float16']
>    powr_output_type = ['float','float2','float4','float8','float16']
>    powr_cpu_func='''
> -static float powr(float x, int y){
> -    if (x<0)
> +static float powr(float x, float y){
> +    if (((x > 0) && (x != +INFINITY)) &&((y == -0) || (y == -0)))

Space after &&. I think you meant (y == +0) here, but I have more comments below about this.

> +        return 1;
> +    else if (((x == +0) || (x == -0)) && ((y <0) || (y == 
> + -INFINITY)))

Space after <

> +        return +INFINITY;
> +    else if (((x == +0) || (x == -0)) && (y > 0))
> +        return +0;
> +    else if (((x == +0) || (x == -0)) && ((y == +0) || (y == -0)))
> +        return NAN;
> +    else if ((x == +1) && ((y == +INFINITY) || (y == -INFINITY)))
> +        return NAN;
> +    else if ((x == +1) && ((y != +INFINITY) && (y != -INFINITY)))
> +        return 1;
> +    else if ((x == +INFINITY) && ((y == +0) || (y == -0)))

This pattern of (y == +0) || (y == -0) is meaningless for a few reasons:

Float == comparison against 0.0f is true if the float is positive or negative 0.0f. There's no need to test for +0.0f and -0.0f separately.

Also, the literals you've used ("+0", "-0") are integers which are implicitly promoted to float, and since there isn't a negative-zero integer representation, they both evaluate to (y == 0.0f)... which as I said already handles both positive and negative zero.

The code should simply be (y == 0.0f). (The expression y == 0.0 implicitly promotes y to a double since 0.0 without the suffix is
double-precision)

> +        return NAN;
> +    else if (isnan(x) || (x < 0))
> +        return NAN;
> +    else if ((x >=  0) && (isnan(y)))
>          return NAN;
>      else
>          return powf(x,y);
> --
> 1.9.1
>
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet