[v5] LOOP-UNROLL: Leverage HAS_SIGNED_ZERO for var expansion

Message ID 20240111085043.1246942-1-pan2.li@intel.com
State Accepted
Headers
Series [v5] LOOP-UNROLL: Leverage HAS_SIGNED_ZERO for var expansion |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Li, Pan2 Jan. 11, 2024, 8:50 a.m. UTC
  From: Pan Li <pan2.li@intel.com>

The insert_var_expansion_initialization depends on the
HONOR_SIGNED_ZEROS to initialize the unrolling variables
to +0.0f when -0.0f and no-signed-option.  Unfortunately,
we should always keep the -0.0f here because:

* The -0.0f is always the correct initial value.
* We need to support the target that always honor signed zero.

Thus, we need to leverage MODE_HAS_SIGNED_ZEROS when initialize
instead of HONOR_SIGNED_ZEROS.  Then the target/backend can
decide to honor the no-signed-zero or not.

We also removed the testcase pr30957-1.c, as it makes undefined behavior
whether the return value is positive or negative.

The below tests are passed for this patch:

* The riscv regression tests.
* The aarch64 regression tests.
* The x86 bootstrap and regression tests.

gcc/ChangeLog:

	* loop-unroll.cc (insert_var_expansion_initialization): Leverage
	MODE_HAS_SIGNED_ZEROS for expansion variable initialization.

gcc/testsuite/ChangeLog:

	* gcc.dg/pr30957-1.c: Remove.

Signed-off-by: Pan Li <pan2.li@intel.com>
---
 gcc/loop-unroll.cc               |  4 ++--
 gcc/testsuite/gcc.dg/pr30957-1.c | 36 --------------------------------
 2 files changed, 2 insertions(+), 38 deletions(-)
 delete mode 100644 gcc/testsuite/gcc.dg/pr30957-1.c
  

Comments

Richard Biener Jan. 11, 2024, 9:21 a.m. UTC | #1
On Thu, Jan 11, 2024 at 9:50 AM <pan2.li@intel.com> wrote:
>
> From: Pan Li <pan2.li@intel.com>
>
> The insert_var_expansion_initialization depends on the
> HONOR_SIGNED_ZEROS to initialize the unrolling variables
> to +0.0f when -0.0f and no-signed-option.  Unfortunately,
> we should always keep the -0.0f here because:
>
> * The -0.0f is always the correct initial value.
> * We need to support the target that always honor signed zero.
>
> Thus, we need to leverage MODE_HAS_SIGNED_ZEROS when initialize
> instead of HONOR_SIGNED_ZEROS.  Then the target/backend can
> decide to honor the no-signed-zero or not.
>
> We also removed the testcase pr30957-1.c, as it makes undefined behavior
> whether the return value is positive or negative.
>
> The below tests are passed for this patch:
>
> * The riscv regression tests.
> * The aarch64 regression tests.
> * The x86 bootstrap and regression tests.

OK

> gcc/ChangeLog:
>
>         * loop-unroll.cc (insert_var_expansion_initialization): Leverage
>         MODE_HAS_SIGNED_ZEROS for expansion variable initialization.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/pr30957-1.c: Remove.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
> ---
>  gcc/loop-unroll.cc               |  4 ++--
>  gcc/testsuite/gcc.dg/pr30957-1.c | 36 --------------------------------
>  2 files changed, 2 insertions(+), 38 deletions(-)
>  delete mode 100644 gcc/testsuite/gcc.dg/pr30957-1.c
>
> diff --git a/gcc/loop-unroll.cc b/gcc/loop-unroll.cc
> index 4176a21e308..bfdfe6c2bb7 100644
> --- a/gcc/loop-unroll.cc
> +++ b/gcc/loop-unroll.cc
> @@ -1855,7 +1855,7 @@ insert_var_expansion_initialization (struct var_to_expand *ve,
>    rtx var, zero_init;
>    unsigned i;
>    machine_mode mode = GET_MODE (ve->reg);
> -  bool honor_signed_zero_p = HONOR_SIGNED_ZEROS (mode);
> +  bool has_signed_zero_p = MODE_HAS_SIGNED_ZEROS (mode);
>
>    if (ve->var_expansions.length () == 0)
>      return;
> @@ -1869,7 +1869,7 @@ insert_var_expansion_initialization (struct var_to_expand *ve,
>      case MINUS:
>        FOR_EACH_VEC_ELT (ve->var_expansions, i, var)
>          {
> -         if (honor_signed_zero_p)
> +         if (has_signed_zero_p)
>             zero_init = simplify_gen_unary (NEG, mode, CONST0_RTX (mode), mode);
>           else
>             zero_init = CONST0_RTX (mode);
> diff --git a/gcc/testsuite/gcc.dg/pr30957-1.c b/gcc/testsuite/gcc.dg/pr30957-1.c
> deleted file mode 100644
> index 564410913ab..00000000000
> --- a/gcc/testsuite/gcc.dg/pr30957-1.c
> +++ /dev/null
> @@ -1,36 +0,0 @@
> -/* { dg-do run { xfail { mmix-*-* } } } */
> -/* We don't (and don't want to) perform this optimisation on soft-float targets,
> -   where each addition is a library call.  /
> -/* { dg-require-effective-target hard_float } */
> -/* -fassociative-math requires -fno-trapping-math and -fno-signed-zeros. */
> -/* { dg-options "-O2 -funroll-loops -fassociative-math -fno-trapping-math -fno-signed-zeros -fvariable-expansion-in-unroller -fdump-rtl-loop2_unroll" } */
> -
> -extern void abort (void);
> -extern void exit (int);
> -
> -float __attribute__((noinline))
> -foo (float d, int n)
> -{
> -  unsigned i;
> -  float accum = d;
> -
> -  for (i = 0; i < n; i++)
> -    accum += d;
> -
> -  return accum;
> -}
> -
> -int
> -main ()
> -{
> -  /* When compiling standard compliant we expect foo to return -0.0.  But the
> -     variable expansion during unrolling optimization (for this testcase enabled
> -     by non-compliant -fassociative-math) instantiates copy(s) of the
> -     accumulator which it initializes with +0.0.  Hence we expect that foo
> -     returns +0.0.  */
> -  if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != 1.0)
> -    abort ();
> -  exit (0);
> -}
> -
> -/* { dg-final { scan-rtl-dump "Expanding Accumulator" "loop2_unroll" { xfail mmix-*-* } } } */
> --
> 2.34.1
>
  
Li, Pan2 Jan. 11, 2024, 10:35 a.m. UTC | #2
Committed, thanks Richard.

Pan

-----Original Message-----
From: Richard Biener <richard.guenther@gmail.com> 
Sent: Thursday, January 11, 2024 5:22 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; jeffreyalaw@gmail.com
Subject: Re: [PATCH v5] LOOP-UNROLL: Leverage HAS_SIGNED_ZERO for var expansion

On Thu, Jan 11, 2024 at 9:50 AM <pan2.li@intel.com> wrote:
>
> From: Pan Li <pan2.li@intel.com>
>
> The insert_var_expansion_initialization depends on the
> HONOR_SIGNED_ZEROS to initialize the unrolling variables
> to +0.0f when -0.0f and no-signed-option.  Unfortunately,
> we should always keep the -0.0f here because:
>
> * The -0.0f is always the correct initial value.
> * We need to support the target that always honor signed zero.
>
> Thus, we need to leverage MODE_HAS_SIGNED_ZEROS when initialize
> instead of HONOR_SIGNED_ZEROS.  Then the target/backend can
> decide to honor the no-signed-zero or not.
>
> We also removed the testcase pr30957-1.c, as it makes undefined behavior
> whether the return value is positive or negative.
>
> The below tests are passed for this patch:
>
> * The riscv regression tests.
> * The aarch64 regression tests.
> * The x86 bootstrap and regression tests.

OK

> gcc/ChangeLog:
>
>         * loop-unroll.cc (insert_var_expansion_initialization): Leverage
>         MODE_HAS_SIGNED_ZEROS for expansion variable initialization.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/pr30957-1.c: Remove.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
> ---
>  gcc/loop-unroll.cc               |  4 ++--
>  gcc/testsuite/gcc.dg/pr30957-1.c | 36 --------------------------------
>  2 files changed, 2 insertions(+), 38 deletions(-)
>  delete mode 100644 gcc/testsuite/gcc.dg/pr30957-1.c
>
> diff --git a/gcc/loop-unroll.cc b/gcc/loop-unroll.cc
> index 4176a21e308..bfdfe6c2bb7 100644
> --- a/gcc/loop-unroll.cc
> +++ b/gcc/loop-unroll.cc
> @@ -1855,7 +1855,7 @@ insert_var_expansion_initialization (struct var_to_expand *ve,
>    rtx var, zero_init;
>    unsigned i;
>    machine_mode mode = GET_MODE (ve->reg);
> -  bool honor_signed_zero_p = HONOR_SIGNED_ZEROS (mode);
> +  bool has_signed_zero_p = MODE_HAS_SIGNED_ZEROS (mode);
>
>    if (ve->var_expansions.length () == 0)
>      return;
> @@ -1869,7 +1869,7 @@ insert_var_expansion_initialization (struct var_to_expand *ve,
>      case MINUS:
>        FOR_EACH_VEC_ELT (ve->var_expansions, i, var)
>          {
> -         if (honor_signed_zero_p)
> +         if (has_signed_zero_p)
>             zero_init = simplify_gen_unary (NEG, mode, CONST0_RTX (mode), mode);
>           else
>             zero_init = CONST0_RTX (mode);
> diff --git a/gcc/testsuite/gcc.dg/pr30957-1.c b/gcc/testsuite/gcc.dg/pr30957-1.c
> deleted file mode 100644
> index 564410913ab..00000000000
> --- a/gcc/testsuite/gcc.dg/pr30957-1.c
> +++ /dev/null
> @@ -1,36 +0,0 @@
> -/* { dg-do run { xfail { mmix-*-* } } } */
> -/* We don't (and don't want to) perform this optimisation on soft-float targets,
> -   where each addition is a library call.  /
> -/* { dg-require-effective-target hard_float } */
> -/* -fassociative-math requires -fno-trapping-math and -fno-signed-zeros. */
> -/* { dg-options "-O2 -funroll-loops -fassociative-math -fno-trapping-math -fno-signed-zeros -fvariable-expansion-in-unroller -fdump-rtl-loop2_unroll" } */
> -
> -extern void abort (void);
> -extern void exit (int);
> -
> -float __attribute__((noinline))
> -foo (float d, int n)
> -{
> -  unsigned i;
> -  float accum = d;
> -
> -  for (i = 0; i < n; i++)
> -    accum += d;
> -
> -  return accum;
> -}
> -
> -int
> -main ()
> -{
> -  /* When compiling standard compliant we expect foo to return -0.0.  But the
> -     variable expansion during unrolling optimization (for this testcase enabled
> -     by non-compliant -fassociative-math) instantiates copy(s) of the
> -     accumulator which it initializes with +0.0.  Hence we expect that foo
> -     returns +0.0.  */
> -  if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != 1.0)
> -    abort ();
> -  exit (0);
> -}
> -
> -/* { dg-final { scan-rtl-dump "Expanding Accumulator" "loop2_unroll" { xfail mmix-*-* } } } */
> --
> 2.34.1
>
  

Patch

diff --git a/gcc/loop-unroll.cc b/gcc/loop-unroll.cc
index 4176a21e308..bfdfe6c2bb7 100644
--- a/gcc/loop-unroll.cc
+++ b/gcc/loop-unroll.cc
@@ -1855,7 +1855,7 @@  insert_var_expansion_initialization (struct var_to_expand *ve,
   rtx var, zero_init;
   unsigned i;
   machine_mode mode = GET_MODE (ve->reg);
-  bool honor_signed_zero_p = HONOR_SIGNED_ZEROS (mode);
+  bool has_signed_zero_p = MODE_HAS_SIGNED_ZEROS (mode);
 
   if (ve->var_expansions.length () == 0)
     return;
@@ -1869,7 +1869,7 @@  insert_var_expansion_initialization (struct var_to_expand *ve,
     case MINUS:
       FOR_EACH_VEC_ELT (ve->var_expansions, i, var)
         {
-	  if (honor_signed_zero_p)
+	  if (has_signed_zero_p)
 	    zero_init = simplify_gen_unary (NEG, mode, CONST0_RTX (mode), mode);
 	  else
 	    zero_init = CONST0_RTX (mode);
diff --git a/gcc/testsuite/gcc.dg/pr30957-1.c b/gcc/testsuite/gcc.dg/pr30957-1.c
deleted file mode 100644
index 564410913ab..00000000000
--- a/gcc/testsuite/gcc.dg/pr30957-1.c
+++ /dev/null
@@ -1,36 +0,0 @@ 
-/* { dg-do run { xfail { mmix-*-* } } } */
-/* We don't (and don't want to) perform this optimisation on soft-float targets,
-   where each addition is a library call.  /
-/* { dg-require-effective-target hard_float } */
-/* -fassociative-math requires -fno-trapping-math and -fno-signed-zeros. */
-/* { dg-options "-O2 -funroll-loops -fassociative-math -fno-trapping-math -fno-signed-zeros -fvariable-expansion-in-unroller -fdump-rtl-loop2_unroll" } */
-
-extern void abort (void);
-extern void exit (int);
-
-float __attribute__((noinline))
-foo (float d, int n)
-{
-  unsigned i;
-  float accum = d;
-
-  for (i = 0; i < n; i++)
-    accum += d;
-
-  return accum;
-}
-
-int
-main ()
-{
-  /* When compiling standard compliant we expect foo to return -0.0.  But the
-     variable expansion during unrolling optimization (for this testcase enabled
-     by non-compliant -fassociative-math) instantiates copy(s) of the
-     accumulator which it initializes with +0.0.  Hence we expect that foo
-     returns +0.0.  */
-  if (__builtin_copysignf (1.0, foo (0.0 / -5.0, 10)) != 1.0)
-    abort ();
-  exit (0);
-}
-
-/* { dg-final { scan-rtl-dump "Expanding Accumulator" "loop2_unroll" { xfail mmix-*-* } } } */