On Thu, Jun 13, 2024 at 3:32 PM Alexandre Oliva <ol...@adacore.com> wrote:
>
>
> The first patch for PR113719 regressed gcc.dg/ipa/iinline-attr.c on
> toolchains configured to --enable-frame-pointer, because the
> optimization node created within handle_optimize_attribute had
> flag_omit_frame_pointer incorrectly set, whereas
> default_optimization_node didn't. With this difference,
> can_inline_edge_by_limits_p flagged an optimization mismatch and we
> refused to inline the function that had a redundant optimization flag
> into one that didn't, which is exactly what is tested for there.
>
> This patch restores the calls to ix86_default_align and
> ix86_recompute_optlev_based_flags that used to be, and ought to be,
> issued during TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE, but preserves the
> intent of the original change, of having those functions called at
> different spots within ix86_option_override_internal. To that end,
> the remaining bits were refactored into a separate function, that was
> in turn adjusted to operate on explicitly-passed opts and opts_set,
> rather than going for their global counterparts.
>
> Regstrapped on x86_64-linux-gnu. Also tested with
> --enable-frame-pointer, and with gcc-13 x-x86-vx7r2, where the problem
> was detected. Ok to install?
LGTM, thanks.
>
>
> for gcc/ChangeLog
>
> PR target/113719
> * config/i386/i386-options.cc
> (ix86_override_options_after_change_1): Add opts and opts_set
> parms, operate on them, after factoring out of...
> (ix86_override_options_after_change): ... this. Restore calls
> of ix86_default_align and ix86_recompute_optlev_based_flags.
> (ix86_option_override_internal): Call the factored-out bits.
> ---
> gcc/config/i386/i386-options.cc | 59
> ++++++++++++++++++++++++++-------------
> 1 file changed, 40 insertions(+), 19 deletions(-)
>
> diff --git a/gcc/config/i386/i386-options.cc b/gcc/config/i386/i386-options.cc
> index f2cecc0e2545b..7fa7f6774e9cf 100644
> --- a/gcc/config/i386/i386-options.cc
> +++ b/gcc/config/i386/i386-options.cc
> @@ -1911,37 +1911,58 @@ ix86_recompute_optlev_based_flags (struct gcc_options
> *opts,
> }
> }
>
> -/* Implement TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook. */
> +/* Implement part of TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook. */
>
> -void
> -ix86_override_options_after_change (void)
> +static void
> +ix86_override_options_after_change_1 (struct gcc_options *opts,
> + struct gcc_options *opts_set)
> {
> +#define OPTS_SET_P(OPTION) opts_set->x_ ## OPTION
> +#define OPTS(OPTION) opts->x_ ## OPTION
> +
> /* 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 ((OPTS_SET_P (flag_unroll_loops))
> + || (OPTS_SET_P (flag_unroll_all_loops)
> + && OPTS (flag_unroll_all_loops)))
> {
> - if (!OPTION_SET_P (ix86_unroll_only_small_loops))
> - ix86_unroll_only_small_loops = 0;
> + if (!OPTS_SET_P (ix86_unroll_only_small_loops))
> + OPTS (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;
> + if (!OPTS_SET_P (flag_web))
> + OPTS (flag_web) = OPTS (flag_unroll_loops);
> + if (!OPTS_SET_P (flag_rename_registers))
> + OPTS (flag_rename_registers) = OPTS (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;
> + if (!OPTS_SET_P (flag_cunroll_grow_size))
> + OPTS (flag_cunroll_grow_size)
> + = (OPTS (flag_unroll_loops)
> + || OPTS (flag_peel_loops)
> + || OPTS (optimize) >= 3);
> }
> else
> {
> - if (!OPTION_SET_P (flag_cunroll_grow_size))
> - flag_cunroll_grow_size = flag_peel_loops || optimize >= 3;
> + if (!OPTS_SET_P (flag_cunroll_grow_size))
> + OPTS (flag_cunroll_grow_size)
> + = (OPTS (flag_peel_loops)
> + || OPTS (optimize) >= 3);
> }
>
> +#undef OPTS
> +#undef OPTS_SET_P
> +}
> +
> +/* Implement TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook. */
> +
> +void
> +ix86_override_options_after_change (void)
> +{
> + ix86_default_align (&global_options);
> +
> + ix86_recompute_optlev_based_flags (&global_options, &global_options_set);
> +
> + ix86_override_options_after_change_1 (&global_options,
> &global_options_set);
> }
>
> /* Clear stack slot assignments remembered from previous functions.
> @@ -2488,7 +2509,7 @@ ix86_option_override_internal (bool main_args_p,
>
> ix86_recompute_optlev_based_flags (opts, opts_set);
>
> - ix86_override_options_after_change ();
> + ix86_override_options_after_change_1 (opts, opts_set);
>
> ix86_tune_cost = processor_cost_table[ix86_tune];
> /* TODO: ix86_cost should be chosen at instruction or function granuality
>
> --
> Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/
> Free Software Activist GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive
--
BR,
Hongtao
