diff mbox series

rs6000: Adjust loop_unroll_adjust to match middle-end change [PR 107692]

Message ID 20221116235429.25268-1-hongyu.wang@intel.com
State New
Headers show
Series rs6000: Adjust loop_unroll_adjust to match middle-end change [PR 107692] | expand

Commit Message

Hongyu Wang Nov. 16, 2022, 11:54 p.m. UTC 
Hi,

r13-3950-g071e428c24ee8c enables O2 small loop unrolling, but it breaks
-fno-unroll-loops for rs6000 with loop_unroll_adjust hook. Adjust the
option handling and target hook accordingly.

Bootstrapped & regtested on powerpc64le-linux-gnu, OK for trunk?

gcc/ChangeLog:

	PR target/107692
	* common/config/rs6000/rs6000-common.cc: Do not enable
	funroll-loops by default.
	* config/rs6000/rs6000.cc (rs6000_override_options_after_change):
	Remove flag override about loop unroll.
	(rs6000_loop_unroll_adjust): Turns off munroll-only-small-loops
	when explicit -f[no-]unroll-[all-]loops is set.
---
 gcc/common/config/rs6000/rs6000-common.cc |  6 +--
 gcc/config/rs6000/rs6000.cc               | 45 ++++++++++-------------
 2 files changed, 22 insertions(+), 29 deletions(-)

Comments

Segher Boessenkool Nov. 18, 2022, 2:46 p.m. UTC | #1 
[ Please cc: me and Ke Wen on rs6000 patches ]

On Thu, Nov 17, 2022 at 07:54:29AM +0800, Hongyu Wang wrote:
> r13-3950-g071e428c24ee8c enables O2 small loop unrolling, but it breaks
> -fno-unroll-loops for rs6000 with loop_unroll_adjust hook. Adjust the
> option handling and target hook accordingly.

NAK.

This is wrong.  -munroll-only-small-loops does not enable loop
unrolling; doing that with a machine flag is completely unmaintainable,
also for people using different targets.

Something in your patch was wrong, please fix that (or revert the
patch).  You should not have to touch config/rs6000/ at all.


Segher
Richard Biener Nov. 22, 2022, 9 a.m. UTC | #2 
On Fri, Nov 18, 2022 at 3:47 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> [ Please cc: me and Ke Wen on rs6000 patches ]
>
> On Thu, Nov 17, 2022 at 07:54:29AM +0800, Hongyu Wang wrote:
> > r13-3950-g071e428c24ee8c enables O2 small loop unrolling, but it breaks
> > -fno-unroll-loops for rs6000 with loop_unroll_adjust hook. Adjust the
> > option handling and target hook accordingly.
>
> NAK.
>
> This is wrong.  -munroll-only-small-loops does not enable loop
> unrolling; doing that with a machine flag is completely unmaintainable,
> also for people using different targets.

I suggested that because doing it fully in the backend by twiddling
-funroll-loops is unmaintainable as well.

> Something in your patch was wrong, please fix that (or revert the
> patch).  You should not have to touch config/rs6000/ at all.

Sure something is wrong, but I think there's the opportunity to
simplify rs6000/ and s390x/, the only other two implementors of
the hook used.

Richard.

>
> Segher
Hongyu Wang Nov. 23, 2022, 1:53 a.m. UTC | #3 
Hi, Segher and Richard

> > Something in your patch was wrong, please fix that (or revert the
> > patch).  You should not have to touch config/rs6000/ at all.
>
> Sure something is wrong, but I think there's the opportunity to
> simplify rs6000/ and s390x/, the only other two implementors of
> the hook used.

If I understand correctly, the wrong part is we should not break the
logic of -funroll-loops and check OPTION_SET_P in
targetm.loop_unroll_adjust to pretend the loop-unrolling is disabled
with -fno-unroll-loops.
I don't have a good idea to resolve this, perhaps add another hook and
check OPTION_SET_P (flag_unroll_loops) && munroll_only_small_loops
there and use that hook in rtl_loop_unroll::gate (), but still it
doesn't work if we want to strictly follow the logic that
-munroll-only-small-loops should not enable loop unrolling.
IMHO the middle-end part with target hook looks quite tricky (and of
course the OPTION_SET_P in the target hook). So Richard if you agree,
I'd like to install the reversion patch posted in
https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606774.html
and move all them to the backend first.
Richard Biener Nov. 23, 2022, 8:02 p.m. UTC | #4 
On Wed, Nov 23, 2022 at 2:53 AM Hongyu Wang <wwwhhhyyy333@gmail.com> wrote:
>
> Hi, Segher and Richard
>
> > > Something in your patch was wrong, please fix that (or revert the
> > > patch).  You should not have to touch config/rs6000/ at all.
> >
> > Sure something is wrong, but I think there's the opportunity to
> > simplify rs6000/ and s390x/, the only other two implementors of
> > the hook used.
>
> If I understand correctly, the wrong part is we should not break the
> logic of -funroll-loops and check OPTION_SET_P in
> targetm.loop_unroll_adjust to pretend the loop-unrolling is disabled
> with -fno-unroll-loops.
> I don't have a good idea to resolve this, perhaps add another hook and
> check OPTION_SET_P (flag_unroll_loops) && munroll_only_small_loops
> there and use that hook in rtl_loop_unroll::gate (), but still it
> doesn't work if we want to strictly follow the logic that
> -munroll-only-small-loops should not enable loop unrolling.
> IMHO the middle-end part with target hook looks quite tricky (and of
> course the OPTION_SET_P in the target hook). So Richard if you agree,
> I'd like to install the reversion patch posted in
> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606774.html
> and move all them to the backend first.

Fine by me.

Richard.

> --
> Regards,
>
> Hongyu, Wang
diff mbox series

Patch

diff --git a/gcc/common/config/rs6000/rs6000-common.cc b/gcc/common/config/rs6000/rs6000-common.cc
index 8e393d08a23..7186d0c309c 100644
--- a/gcc/common/config/rs6000/rs6000-common.cc
+++ b/gcc/common/config/rs6000/rs6000-common.cc
@@ -34,9 +34,9 @@  static const struct default_options rs6000_option_optimization_table[] =
     { OPT_LEVELS_ALL, OPT_fsplit_wide_types_early, NULL, 1 },
     /* Enable -fsched-pressure for first pass instruction scheduling.  */
     { OPT_LEVELS_1_PLUS, OPT_fsched_pressure, NULL, 1 },
-    /* Enable -munroll-only-small-loops with -funroll-loops to unroll small
-       loops at -O2 and above by default.  */
-    { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_funroll_loops, NULL, 1 },
+
+    /* Enable -munroll-only-small-loops to unroll small loops at -O2 and
+       above by default.  */
     { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_munroll_only_small_loops, NULL, 1 },
 
     /* -frename-registers leads to non-optimal codegen and performance
diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index a85d7630b41..26e41b4b3b0 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -3392,22 +3392,6 @@  rs6000_md_asm_adjust (vec<rtx> & /*outputs*/, vec<rtx> & /*inputs*/,
 static void
 rs6000_override_options_after_change (void)
 {
-  /* Explicit -funroll-loops turns -munroll-only-small-loops off, and
-     turns -frename-registers on.  */
-  if ((OPTION_SET_P (flag_unroll_loops) && flag_unroll_loops)
-       || (OPTION_SET_P (flag_unroll_all_loops)
-	   && flag_unroll_all_loops))
-    {
-      if (!OPTION_SET_P (unroll_only_small_loops))
-	unroll_only_small_loops = 0;
-      if (!OPTION_SET_P (flag_rename_registers))
-	flag_rename_registers = 1;
-      if (!OPTION_SET_P (flag_cunroll_grow_size))
-	flag_cunroll_grow_size = 1;
-    }
-  else if (!OPTION_SET_P (flag_cunroll_grow_size))
-    flag_cunroll_grow_size = flag_peel_loops || optimize >= 3;
-
   /* If we are inserting ROP-protect instructions, disable shrink wrap.  */
   if (rs6000_rop_protect)
     flag_shrink_wrap = 0;
@@ -5540,17 +5524,26 @@  rs6000_cost_data::finish_cost (const vector_costs *scalar_costs)
 static unsigned
 rs6000_loop_unroll_adjust (unsigned nunroll, struct loop *loop)
 {
-   if (unroll_only_small_loops)
-    {
-      /* TODO: These are hardcoded values right now.  We probably should use
-	 a PARAM here.  */
-      if (loop->ninsns <= 6)
-	return MIN (4, nunroll);
-      if (loop->ninsns <= 10)
-	return MIN (2, nunroll);
+  if (!(flag_unroll_loops || flag_unroll_all_loops
+	|| loop->unroll))
+  {
+    unsigned nunroll_orig = nunroll;
+    nunroll = 1;
+    /* Any explicit -f{no-}unroll-{all-}loops turns off
+       -munroll-only-small-loops.  */
+    if (unroll_only_small_loops
+	&& !OPTION_SET_P (flag_unroll_loops))
+      {
+	/* TODO: These are hardcoded values right now.  We probably should use
+	   a PARAM here.  */
+	if (loop->ninsns <= 6)
+	  return MIN (4, nunroll_orig);
+	if (loop->ninsns <= 10)
+	  return MIN (2, nunroll_orig);
 
-      return 0;
-    }
+	return 0;
+      }
+  }
 
   return nunroll;
 }