fix a powr function issue in cpu compiler math

Submitted by Meng, Mengmeng on July 19, 2015, 1:33 p.m.

Details

Message ID 1437312825-17462-1-git-send-email-mengmeng.meng@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Meng, Mengmeng July 19, 2015, 1:33 p.m.
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(-)

Patch hide | download patch | download mbox

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_type,[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)))
+        return 1;
+    else if (((x == +0) || (x == -0)) && ((y <0) || (y == -INFINITY)))
+        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)))
+        return NAN;
+    else if (isnan(x) || (x < 0))
+        return NAN;
+    else if ((x >=  0) && (isnan(y)))
         return NAN;
     else
         return powf(x,y);

Comments

On Sun, Jul 19, 2015 at 6:33 AM, Meng Mengmeng <mengmeng.meng@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_type,[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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
Thanks your comments. I'll send out v2 to fix the issues.

-----Original Message-----
From: Matt Turner [mailto:mattst88@gmail.com] 

Sent: Thursday, July 30, 2015 1:37 AM
To: Meng, Mengmeng
Cc: beignet@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@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@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/beignet