Re: [PATCH] Optimize macro: make it more predictable

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Oct 23, 2020 at 1:47 PM Martin Liška <mliska@suse.cz> wrote:
>
> Hey.
>
> This is a follow-up of the discussion that happened in thread about no_stack_protector
> attribute: https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545916.html
>
> The current optimize attribute works in the following way:
> - 1) we take current global_options as base
> - 2) maybe_default_options is called for the currently selected optimization level, which
>       means all rules in default_options_table are executed
> - 3) attribute values are applied (via decode_options)
>
> So the step 2) is problematic: in case of -O2 -fno-omit-frame-pointer and __attribute__((optimize("-fno-stack-protector")))
> ends basically with -O2 -fno-stack-protector because -fno-omit-frame-pointer is default:
>      /* -O1 and -Og optimizations.  */
>      { OPT_LEVELS_1_PLUS, OPT_fomit_frame_pointer, NULL, 1 },
>
> My patch handled and the current optimize attribute really behaves that same as appending attribute value
> to the command line. So far so good. We should also reflect that in documentation entry which is quite
> vague right now:
>
> """
> The optimize attribute is used to specify that a function is to be compiled with different optimization options than specified on the command line.
> """
>
> and we may want to handle -Ox in the attribute in a special way. I guess many macro/pragma users expect that
>
> -O2 -ftree-vectorize and __attribute__((optimize(1))) will end with -O1 and not
> with -ftree-vectorize -O1 ?

Hmm.  I guess the only two reasonable options are to append to the active set
and thus end up with -ftree-vectorize -O1 or to start from an empty set and thus
end up with -O1.

Maybe we can have

@item optimize (@var{level}, @dots{})

reset everything to plain -On and

@item optimize (@var{string}, @dots{})

append?  So optimize("O1") will end up with -O2 -ftree-vectorize -O1 and
optimize(1) with -O1?  How do we handle

void __attribute__((optimize(1),optimize("ftree-vectorize")))

thus two optimize attributes?

> I'm also planning to take a look at the target macro/attribute, I expect similar problems:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97469
>
> Thoughts?
> Thanks,
> Martin
>
> gcc/c-family/ChangeLog:
>
>         * c-common.c (parse_optimize_options): Decoded attribute options
>         with the ones that were already set on the command line.
>
> gcc/ChangeLog:
>
>         * toplev.c (toplev::main): Save decoded Optimization options.
>         * toplev.h (save_opt_decoded_options): New.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/i386/avx512er-vrsqrt28ps-3.c: Disable -ffast-math.
>         * gcc.target/i386/avx512er-vrsqrt28ps-5.c: Likewise.
> ---
>   gcc/c-family/c-common.c                           | 15 ++++++++++++++-
>   .../gcc.target/i386/avx512er-vrsqrt28ps-3.c       |  2 +-
>   .../gcc.target/i386/avx512er-vrsqrt28ps-5.c       |  2 +-
>   gcc/toplev.c                                      |  8 ++++++++
>   gcc/toplev.h                                      |  1 +
>   5 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index e16ca3894bc..d4342e93d0a 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -5727,10 +5727,23 @@ parse_optimize_options (tree args, bool attr_p)
>         j++;
>       }
>     decoded_options_count = j;
> +
> +  /* Merge the decoded options with save_decoded_options.  */
> +  unsigned save_opt_count = save_opt_decoded_options.length ();
> +  unsigned merged_decoded_options_count = save_opt_count + decoded_options_count;
> +  cl_decoded_option *merged_decoded_options
> +    = XNEWVEC (cl_decoded_option, merged_decoded_options_count);
> +
> +  for (unsigned i = 0; i < save_opt_count; ++i)
> +    merged_decoded_options[i] = save_opt_decoded_options[i];
> +  for (unsigned i = 0; i < decoded_options_count; ++i)
> +    merged_decoded_options[save_opt_count + i] = decoded_options[i];
> +
>     /* And apply them.  */
>     decode_options (&global_options, &global_options_set,
> -                 decoded_options, decoded_options_count,
> +                 merged_decoded_options, merged_decoded_options_count,
>                   input_location, global_dc, NULL);
> +  free (decoded_options);
>
>     targetm.override_options_after_change();
>
> diff --git a/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-3.c b/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-3.c
> index 1ba8172d6e3..40aefb50844 100644
> --- a/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-3.c
> +++ b/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-3.c
> @@ -8,7 +8,7 @@
>   #define MAX 1000
>   #define EPS 0.00001
>
> -__attribute__ ((noinline, optimize (1)))
> +__attribute__ ((noinline, optimize (1, "-fno-fast-math")))
>   void static
>   compute_rsqrt_ref (float *a, float *r)
>   {
> diff --git a/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-5.c b/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-5.c
> index e067a81e562..498f4d50aa5 100644
> --- a/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-5.c
> +++ b/gcc/testsuite/gcc.target/i386/avx512er-vrsqrt28ps-5.c
> @@ -8,7 +8,7 @@
>   #define MAX 1000
>   #define EPS 0.00001
>
> -__attribute__ ((noinline, optimize (1)))
> +__attribute__ ((noinline, optimize (1, "-fno-fast-math")))
>   void static
>   compute_sqrt_ref (float *a, float *r)
>   {
> diff --git a/gcc/toplev.c b/gcc/toplev.c
> index 8c1e1e1f44f..27e4a1bc485 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -123,6 +123,9 @@ static bool no_backend;
>   struct cl_decoded_option *save_decoded_options;
>   unsigned int save_decoded_options_count;
>
> +/* Vector of saved Optimization decoded command line options.  */
> +auto_vec<cl_decoded_option> save_opt_decoded_options;
> +
>   /* Used to enable -fvar-tracking, -fweb and -frename-registers according
>      to optimize in process_options ().  */
>   #define AUTODETECT_VALUE 2
> @@ -2430,6 +2433,11 @@ toplev::main (int argc, char **argv)
>                                                 &save_decoded_options,
>                                                 &save_decoded_options_count);
>
> +  /* Save Optimization decoded options.  */
> +  for (unsigned i = 0; i < save_decoded_options_count; ++i)
> +    if (cl_options[save_decoded_options[i].opt_index].flags & CL_OPTIMIZATION)
> +      save_opt_decoded_options.safe_push (save_decoded_options[i]);
> +
>     /* Perform language-specific options initialization.  */
>     lang_hooks.init_options (save_decoded_options_count, save_decoded_options);
>
> diff --git a/gcc/toplev.h b/gcc/toplev.h
> index d6c316962b0..627312f85f5 100644
> --- a/gcc/toplev.h
> +++ b/gcc/toplev.h
> @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3.  If not see
>   /* Decoded options, and number of such options.  */
>   extern struct cl_decoded_option *save_decoded_options;
>   extern unsigned int save_decoded_options_count;
> +extern auto_vec<cl_decoded_option> save_opt_decoded_options;
>
>   class timer;
>
> --
> 2.28.0
>