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 |
[ 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
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; }