Re: [PATCH] i386: Only enable small loop unrolling in backend [PR 107602]

2022-11-22 Thread Hongyu Wang via Gcc-patches 
Hi Jeff,

> The reversion of the loop-init.cc changes is fine.  The x86 maintainers
> will need to chime in on the rest.  Consider installing the loop-init.cc
> reversion immediately as the current state has regressed s390 and
> potentially other targets.

I've posted a patch in
https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606478.html
to change the rs6000 target and there is a discussion ongoing. If we
draw a conclusion that we only want to make changes in the backend, I
will install this one.

-- 
Regards,

Hongyu, Wang

Re: [PATCH] i386: Only enable small loop unrolling in backend [PR 107602]

2022-11-21 Thread Hongtao Liu via Gcc-patches 
On Tue, Nov 22, 2022 at 1:41 AM Jeff Law via Gcc-patches
 wrote:
>
>
> On 11/18/22 23:25, Hongyu Wang via Gcc-patches wrote:
> > Hi,
> >
> > Followed by the discussion in pr107602, -munroll-only-small-loops
> > Does not turns on/off -funroll-loops, and current check in
> > pass_rtl_unroll_loops::gate would cause -funroll-loops do not take
> > effect. Revert the change about targetm.loop_unroll_adjust and apply
> > the backend option change to strictly follow the rule that
> > -funroll-loops takes full control of loop unrolling, and
> > munroll-only-small-loops just change its behavior to unroll small size
> > loops.
> >
> > Bootstrapped and regtested on x86-64-pc-linux-gnu.
> >
> > Ok for trunk?
> >
> > gcc/ChangeLog:
> >
> >   PR target/107602
> >   * common/config/i386/i386-common.cc (ix86_optimization_table):
> >   Enable loop unroll O2, disable -fweb and -frename-registers
> >   by default.
> >   * config/i386/i386-options.cc
> >   (ix86_override_options_after_change):
> >   Disable small loop unroll when funroll-loops enabled, reset
> >   cunroll_grow_size when it is not explicitly enabled.
> >   (ix86_option_override_internal): Call
> >   ix86_override_options_after_change instead of calling
> >   ix86_recompute_optlev_based_flags and ix86_default_align
> >   separately.
> >   * config/i386/i386.cc (ix86_loop_unroll_adjust): Adjust unroll
> >   factor if -munroll-only-small-loops enabled.
> >   * loop-init.cc (pass_rtl_unroll_loops::gate): Do not enable
> >   loop unrolling for -O2-speed.
> >   (pass_rtl_unroll_loops::execute): Rmove
> >   targetm.loop_unroll_adjust check.
> The reversion of the loop-init.cc changes is fine.  The x86 maintainers
> will need to chime in on the rest.  Consider installing the loop-init.cc
> reversion immediately as the current state has regressed s390 and
> potentially other targets.
x86 part is ok.
>
>
> jeff
>


-- 
BR,
Hongtao

Re: [PATCH] i386: Only enable small loop unrolling in backend [PR 107602]

2022-11-21 Thread Jeff Law via Gcc-patches 



On 11/18/22 23:25, Hongyu Wang via Gcc-patches wrote:

Hi,

Followed by the discussion in pr107602, -munroll-only-small-loops
Does not turns on/off -funroll-loops, and current check in
pass_rtl_unroll_loops::gate would cause -funroll-loops do not take
effect. Revert the change about targetm.loop_unroll_adjust and apply
the backend option change to strictly follow the rule that
-funroll-loops takes full control of loop unrolling, and
munroll-only-small-loops just change its behavior to unroll small size
loops.

Bootstrapped and regtested on x86-64-pc-linux-gnu.

Ok for trunk?

gcc/ChangeLog:

PR target/107602
* common/config/i386/i386-common.cc (ix86_optimization_table):
Enable loop unroll O2, disable -fweb and -frename-registers
by default.
* config/i386/i386-options.cc
(ix86_override_options_after_change):
Disable small loop unroll when funroll-loops enabled, reset
cunroll_grow_size when it is not explicitly enabled.
(ix86_option_override_internal): Call
ix86_override_options_after_change instead of calling
ix86_recompute_optlev_based_flags and ix86_default_align
separately.
* config/i386/i386.cc (ix86_loop_unroll_adjust): Adjust unroll
factor if -munroll-only-small-loops enabled.
* loop-init.cc (pass_rtl_unroll_loops::gate): Do not enable
loop unrolling for -O2-speed.
(pass_rtl_unroll_loops::execute): Rmove
targetm.loop_unroll_adjust check.
The reversion of the loop-init.cc changes is fine.  The x86 maintainers 
will need to chime in on the rest.  Consider installing the loop-init.cc 
reversion immediately as the current state has regressed s390 and 
potentially other targets.



jeff

Re: [PATCH] i386: Only enable small loop unrolling in backend [PR 107602]

2022-11-20 Thread Hongyu Wang via Gcc-patches 
> It's not necessarily right. unroll_factor will be set as 1 when
> -fno-unroll-loops, which is exactly -fno-unroll-loops means.

Not that exactly, -fno-unroll-loops previously will prevent the pass
from running, and on the current trunk the pass still runs.
Actually I think the implementation on trunk is a bit tricky since we
use a target hook without return value to execute the pass. Also the
logic !OPTION_SET_P looks quite like a workaround. IMHO this is also
not good for maintenance.

Hongtao Liu via Gcc-patches  于2022年11月21日周一 09:33写道：
>
> On Mon, Nov 21, 2022 at 9:01 AM Liu, Hongtao via Gcc-patches
>  wrote:
> >
> >
> >
> > > -Original Message-
> > > From: Wang, Hongyu 
> > > Sent: Saturday, November 19, 2022 2:26 PM
> > > To: gcc-patches@gcc.gnu.org
> > > Cc: richard.guent...@gmail.com; ubiz...@gmail.com; Liu, Hongtao
> > > 
> > > Subject: [PATCH] i386: Only enable small loop unrolling in backend [PR 
> > > 107602]
> > >
> > > Hi,
> > >
> > > Followed by the discussion in pr107602, -munroll-only-small-loops Does not
> > PR107692?
> > > turns on/off -funroll-loops, and current check in 
> > > pass_rtl_unroll_loops::gate
> > > would cause -funroll-loops do not take effect. Revert the change about
> It's not necessarily right. unroll_factor will be set as 1 when
> -fno-unroll-loops, which is exactly -fno-unroll-loops means.
> > > targetm.loop_unroll_adjust and apply the backend option change to strictly
> > > follow the rule that -funroll-loops takes full control of loop unrolling, 
> > > and
> > > munroll-only-small-loops just change its behavior to unroll small size 
> > > loops.
> > >
> > > Bootstrapped and regtested on x86-64-pc-linux-gnu.
> > >
> > > Ok for trunk?
> > >
> > > gcc/ChangeLog:
> > >
> > >   PR target/107602
> > >   * common/config/i386/i386-common.cc (ix86_optimization_table):
> > >   Enable loop unroll O2, disable -fweb and -frename-registers
> > >   by default.
> > >   * config/i386/i386-options.cc
> > >   (ix86_override_options_after_change):
> > >   Disable small loop unroll when funroll-loops enabled, reset
> > >   cunroll_grow_size when it is not explicitly enabled.
> > >   (ix86_option_override_internal): Call
> > >   ix86_override_options_after_change instead of calling
> > >   ix86_recompute_optlev_based_flags and ix86_default_align
> > >   separately.
> > >   * config/i386/i386.cc (ix86_loop_unroll_adjust): Adjust unroll
> > >   factor if -munroll-only-small-loops enabled.
> > >   * loop-init.cc (pass_rtl_unroll_loops::gate): Do not enable
> > >   loop unrolling for -O2-speed.
> > >   (pass_rtl_unroll_loops::execute): Rmove
> > >   targetm.loop_unroll_adjust check.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > >   PR target/107602
> > >   * gcc.target/i386/pr86270.c: Add -fno-unroll-loops.
> > >   * gcc.target/i386/pr93002.c: Likewise.
> > > ---
> > >  gcc/common/config/i386/i386-common.cc   |  8 ++
> > >  gcc/config/i386/i386-options.cc | 34 ++---
> > >  gcc/config/i386/i386.cc | 18 -
> > >  gcc/loop-init.cc| 11 +++-
> > >  gcc/testsuite/gcc.target/i386/pr86270.c |  2 +-
> > > gcc/testsuite/gcc.target/i386/pr93002.c |  2 +-
> > >  6 files changed, 49 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/gcc/common/config/i386/i386-common.cc
> > > b/gcc/common/config/i386/i386-common.cc
> > > index 6ce2a588adc..660a977b68b 100644
> > > --- a/gcc/common/config/i386/i386-common.cc
> > > +++ b/gcc/common/config/i386/i386-common.cc
> > > @@ -1808,7 +1808,15 @@ static const struct default_options
> > > ix86_option_optimization_table[] =
> > >  /* The STC algorithm produces the smallest code at -Os, for x86.  */
> > >  { OPT_LEVELS_2_PLUS, OPT_freorder_blocks_algorithm_, NULL,
> > >REORDER_BLOCKS_ALGORITHM_STC },
> > > +
> > > +/* Turn on -funroll-loops with -munroll-only-small-loops to enable 
> > > small
> > > +   loop unrolling at -O2.  */
> > > +{ OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_funroll_loops, NULL, 1 },
> > >  { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_munroll_only_small_loops, NULL,
> > > 1 },
> > > +/* Turns off -frename-registers and -fweb which are enabled by
> > > +   funroll-loops.  */
> > > +{ OPT_LEVELS_ALL, OPT_frename_registers, NULL, 0 },
> > > +{ OPT_LEVELS_ALL, OPT_fweb, NULL, 0 },
> > >  /* Turn off -fschedule-insns by default.  It tends to make the
> > > problem with not enough registers even worse.  */
> > >  { OPT_LEVELS_ALL, OPT_fschedule_insns, NULL, 0 }, diff --git
> > > a/gcc/config/i386/i386-options.cc b/gcc/config/i386/i386-options.cc index
> > > e5c77f3a84d..bc1d36e36a8 100644
> > > --- a/gcc/config/i386/i386-options.cc
> > > +++ b/gcc/config/i386/i386-options.cc
> > > @@ -1838,8 +1838,37 @@ ix86_recompute_optlev_based_flags (struct
> > > gcc_options *opts,  void  ix86_override_options_after_change (void)  {
> >

Re: [PATCH] i386: Only enable small loop unrolling in backend [PR 107602]

2022-11-20 Thread Hongtao Liu via Gcc-patches 
On Mon, Nov 21, 2022 at 9:01 AM Liu, Hongtao via Gcc-patches
 wrote:
>
>
>
> > -Original Message-
> > From: Wang, Hongyu 
> > Sent: Saturday, November 19, 2022 2:26 PM
> > To: gcc-patches@gcc.gnu.org
> > Cc: richard.guent...@gmail.com; ubiz...@gmail.com; Liu, Hongtao
> > 
> > Subject: [PATCH] i386: Only enable small loop unrolling in backend [PR 
> > 107602]
> >
> > Hi,
> >
> > Followed by the discussion in pr107602, -munroll-only-small-loops Does not
> PR107692?
> > turns on/off -funroll-loops, and current check in 
> > pass_rtl_unroll_loops::gate
> > would cause -funroll-loops do not take effect. Revert the change about
It's not necessarily right. unroll_factor will be set as 1 when
-fno-unroll-loops, which is exactly -fno-unroll-loops means.
> > targetm.loop_unroll_adjust and apply the backend option change to strictly
> > follow the rule that -funroll-loops takes full control of loop unrolling, 
> > and
> > munroll-only-small-loops just change its behavior to unroll small size 
> > loops.
> >
> > Bootstrapped and regtested on x86-64-pc-linux-gnu.
> >
> > Ok for trunk?
> >
> > gcc/ChangeLog:
> >
> >   PR target/107602
> >   * common/config/i386/i386-common.cc (ix86_optimization_table):
> >   Enable loop unroll O2, disable -fweb and -frename-registers
> >   by default.
> >   * config/i386/i386-options.cc
> >   (ix86_override_options_after_change):
> >   Disable small loop unroll when funroll-loops enabled, reset
> >   cunroll_grow_size when it is not explicitly enabled.
> >   (ix86_option_override_internal): Call
> >   ix86_override_options_after_change instead of calling
> >   ix86_recompute_optlev_based_flags and ix86_default_align
> >   separately.
> >   * config/i386/i386.cc (ix86_loop_unroll_adjust): Adjust unroll
> >   factor if -munroll-only-small-loops enabled.
> >   * loop-init.cc (pass_rtl_unroll_loops::gate): Do not enable
> >   loop unrolling for -O2-speed.
> >   (pass_rtl_unroll_loops::execute): Rmove
> >   targetm.loop_unroll_adjust check.
> >
> > gcc/testsuite/ChangeLog:
> >
> >   PR target/107602
> >   * gcc.target/i386/pr86270.c: Add -fno-unroll-loops.
> >   * gcc.target/i386/pr93002.c: Likewise.
> > ---
> >  gcc/common/config/i386/i386-common.cc   |  8 ++
> >  gcc/config/i386/i386-options.cc | 34 ++---
> >  gcc/config/i386/i386.cc | 18 -
> >  gcc/loop-init.cc| 11 +++-
> >  gcc/testsuite/gcc.target/i386/pr86270.c |  2 +-
> > gcc/testsuite/gcc.target/i386/pr93002.c |  2 +-
> >  6 files changed, 49 insertions(+), 26 deletions(-)
> >
> > diff --git a/gcc/common/config/i386/i386-common.cc
> > b/gcc/common/config/i386/i386-common.cc
> > index 6ce2a588adc..660a977b68b 100644
> > --- a/gcc/common/config/i386/i386-common.cc
> > +++ b/gcc/common/config/i386/i386-common.cc
> > @@ -1808,7 +1808,15 @@ static const struct default_options
> > ix86_option_optimization_table[] =
> >  /* The STC algorithm produces the smallest code at -Os, for x86.  */
> >  { OPT_LEVELS_2_PLUS, OPT_freorder_blocks_algorithm_, NULL,
> >REORDER_BLOCKS_ALGORITHM_STC },
> > +
> > +/* Turn on -funroll-loops with -munroll-only-small-loops to enable 
> > small
> > +   loop unrolling at -O2.  */
> > +{ OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_funroll_loops, NULL, 1 },
> >  { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_munroll_only_small_loops, NULL,
> > 1 },
> > +/* Turns off -frename-registers and -fweb which are enabled by
> > +   funroll-loops.  */
> > +{ OPT_LEVELS_ALL, OPT_frename_registers, NULL, 0 },
> > +{ OPT_LEVELS_ALL, OPT_fweb, NULL, 0 },
> >  /* Turn off -fschedule-insns by default.  It tends to make the
> > problem with not enough registers even worse.  */
> >  { OPT_LEVELS_ALL, OPT_fschedule_insns, NULL, 0 }, diff --git
> > a/gcc/config/i386/i386-options.cc b/gcc/config/i386/i386-options.cc index
> > e5c77f3a84d..bc1d36e36a8 100644
> > --- a/gcc/config/i386/i386-options.cc
> > +++ b/gcc/config/i386/i386-options.cc
> > @@ -1838,8 +1838,37 @@ ix86_recompute_optlev_based_flags (struct
> > gcc_options *opts,  void  ix86_override_options_after_change (void)  {
> > +  /* Default align_* from the processor table.  */
> >ix86_default_align (_options);
> > +
> >ix86_recompute_optlev_based_flags (_options, _options_set);
> > +
> > +  /* Disable unrolling small loops when there's explicit
> > + -f{,no}unroll-loop.  */
> > +  if ((OPTION_SET_P (flag_unroll_loops))
> > + || (OPTION_SET_P (flag_unroll_all_loops)
> > +  && flag_unroll_all_loops))
> > +{
> > +  if (!OPTION_SET_P (ix86_unroll_only_small_loops))
> > + ix86_unroll_only_small_loops = 0;
> > +  /* Re-enable -frename-registers and -fweb if funroll-loops
> > +  enabled.  */
> > +  if (!OPTION_SET_P (flag_web))
> > + flag_web = flag_unroll_loops;
> > +  if (!OPTION_SET_P (flag_rename_registers))

RE: [PATCH] i386: Only enable small loop unrolling in backend [PR 107602]

2022-11-20 Thread Liu, Hongtao via Gcc-patches 



> -Original Message-
> From: Wang, Hongyu 
> Sent: Saturday, November 19, 2022 2:26 PM
> To: gcc-patches@gcc.gnu.org
> Cc: richard.guent...@gmail.com; ubiz...@gmail.com; Liu, Hongtao
> 
> Subject: [PATCH] i386: Only enable small loop unrolling in backend [PR 107602]
> 
> Hi,
> 
> Followed by the discussion in pr107602, -munroll-only-small-loops Does not
PR107692?
> turns on/off -funroll-loops, and current check in pass_rtl_unroll_loops::gate
> would cause -funroll-loops do not take effect. Revert the change about
> targetm.loop_unroll_adjust and apply the backend option change to strictly
> follow the rule that -funroll-loops takes full control of loop unrolling, and
> munroll-only-small-loops just change its behavior to unroll small size loops.
> 
> Bootstrapped and regtested on x86-64-pc-linux-gnu.
> 
> Ok for trunk?
> 
> gcc/ChangeLog:
> 
>   PR target/107602
>   * common/config/i386/i386-common.cc (ix86_optimization_table):
>   Enable loop unroll O2, disable -fweb and -frename-registers
>   by default.
>   * config/i386/i386-options.cc
>   (ix86_override_options_after_change):
>   Disable small loop unroll when funroll-loops enabled, reset
>   cunroll_grow_size when it is not explicitly enabled.
>   (ix86_option_override_internal): Call
>   ix86_override_options_after_change instead of calling
>   ix86_recompute_optlev_based_flags and ix86_default_align
>   separately.
>   * config/i386/i386.cc (ix86_loop_unroll_adjust): Adjust unroll
>   factor if -munroll-only-small-loops enabled.
>   * loop-init.cc (pass_rtl_unroll_loops::gate): Do not enable
>   loop unrolling for -O2-speed.
>   (pass_rtl_unroll_loops::execute): Rmove
>   targetm.loop_unroll_adjust check.
> 
> gcc/testsuite/ChangeLog:
> 
>   PR target/107602
>   * gcc.target/i386/pr86270.c: Add -fno-unroll-loops.
>   * gcc.target/i386/pr93002.c: Likewise.
> ---
>  gcc/common/config/i386/i386-common.cc   |  8 ++
>  gcc/config/i386/i386-options.cc | 34 ++---
>  gcc/config/i386/i386.cc | 18 -
>  gcc/loop-init.cc| 11 +++-
>  gcc/testsuite/gcc.target/i386/pr86270.c |  2 +-
> gcc/testsuite/gcc.target/i386/pr93002.c |  2 +-
>  6 files changed, 49 insertions(+), 26 deletions(-)
> 
> diff --git a/gcc/common/config/i386/i386-common.cc
> b/gcc/common/config/i386/i386-common.cc
> index 6ce2a588adc..660a977b68b 100644
> --- a/gcc/common/config/i386/i386-common.cc
> +++ b/gcc/common/config/i386/i386-common.cc
> @@ -1808,7 +1808,15 @@ static const struct default_options
> ix86_option_optimization_table[] =
>  /* The STC algorithm produces the smallest code at -Os, for x86.  */
>  { OPT_LEVELS_2_PLUS, OPT_freorder_blocks_algorithm_, NULL,
>REORDER_BLOCKS_ALGORITHM_STC },
> +
> +/* Turn on -funroll-loops with -munroll-only-small-loops to enable small
> +   loop unrolling at -O2.  */
> +{ OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_funroll_loops, NULL, 1 },
>  { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_munroll_only_small_loops, NULL,
> 1 },
> +/* Turns off -frename-registers and -fweb which are enabled by
> +   funroll-loops.  */
> +{ OPT_LEVELS_ALL, OPT_frename_registers, NULL, 0 },
> +{ OPT_LEVELS_ALL, OPT_fweb, NULL, 0 },
>  /* Turn off -fschedule-insns by default.  It tends to make the
> problem with not enough registers even worse.  */
>  { OPT_LEVELS_ALL, OPT_fschedule_insns, NULL, 0 }, diff --git
> a/gcc/config/i386/i386-options.cc b/gcc/config/i386/i386-options.cc index
> e5c77f3a84d..bc1d36e36a8 100644
> --- a/gcc/config/i386/i386-options.cc
> +++ b/gcc/config/i386/i386-options.cc
> @@ -1838,8 +1838,37 @@ ix86_recompute_optlev_based_flags (struct
> gcc_options *opts,  void  ix86_override_options_after_change (void)  {
> +  /* Default align_* from the processor table.  */
>ix86_default_align (_options);
> +
>ix86_recompute_optlev_based_flags (_options, _options_set);
> +
> +  /* Disable unrolling small loops when there's explicit
> + -f{,no}unroll-loop.  */
> +  if ((OPTION_SET_P (flag_unroll_loops))
> + || (OPTION_SET_P (flag_unroll_all_loops)
> +  && flag_unroll_all_loops))
> +{
> +  if (!OPTION_SET_P (ix86_unroll_only_small_loops))
> + ix86_unroll_only_small_loops = 0;
> +  /* Re-enable -frename-registers and -fweb if funroll-loops
> +  enabled.  */
> +  if (!OPTION_SET_P (flag_web))
> + flag_web = flag_unroll_loops;
> +  if (!OPTION_SET_P (flag_rename_registers))
> + flag_rename_registers = flag_unroll_loops;
> +  /* -fcunroll-grow-size default follws -f[no]-unroll-loops.  */
> +  if (!OPTION_SET_P (flag_cunroll_grow_size))
> + flag_cunroll_grow_size = flag_unroll_loops
> +  || flag_peel_loops
> +  || optimize >= 3;
> +}
> +  else
> +{
> +  if (!OPTION_SET_P (flag_cunroll_grow_size))
> +