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

["Martin Liška" <mliska@suse.cz>]

On 11/3/20 2:27 PM, Richard Biener wrote:
> 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

Hello.

To be honest I don't like it much, it can be quite confusing for uses of the GCC.

> 
> void __attribute__((optimize(1),optimize("ftree-vectorize")))
> 
> thus two optimize attributes?

Yes, parse_optimize_options is called twice for the situation.
I'm going to reply to Jakub's email ...

> 
>> 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
>>