rs6000: Adjust loop_unroll_adjust to match middle-end change [PR 107692]
Checks
Commit Message
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
[ 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
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
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.
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
@@ -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
@@ -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;
}