On 10/23/20 1:47 PM, Martin Liška wrote:
> Hey.

Hello.

I deferred the patch for GCC 12. Since the time, I messed up with options
I feel more familiar with the option handling. So ...

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

^^^ all these are still valid arguments, plus I'm adding a new test-case that tests that.

> 
> """
> The optimize attribute is used to specify that a function is to be compiled with different optimization options than specified on the command line.
> """

I addressed that with documentation changes, should be more clear to users. Moreover, I noticed that we declare 'optimize' attribute
as something not for a production use:

"The optimize attribute should be used for debugging purposes only. It is not suitable in production code."

Are we sure about the statement? I know that e.g. glibc uses that.

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

The situation with 'target' attribute is different. When parsing the attribute, we intentionally drop all existing target flags:

$ cat -n gcc/config/i386/i386-options.c
...
   1245                if (opt == IX86_FUNCTION_SPECIFIC_ARCH)
   1246                  {
   1247                    /* If arch= is set,  clear all bits in x_ix86_isa_flags,
   1248                       except for ISA_64BIT, ABI_64, ABI_X32, and CODE16
   1249                       and all bits in x_ix86_isa_flags2.  */
   1250                    opts->x_ix86_isa_flags &= (OPTION_MASK_ISA_64BIT
   1251                                               | OPTION_MASK_ABI_64
   1252                                               | OPTION_MASK_ABI_X32
   1253                                               | OPTION_MASK_CODE16);
   1254                    opts->x_ix86_isa_flags_explicit &= (OPTION_MASK_ISA_64BIT
   1255                                                        | OPTION_MASK_ABI_64
   1256                                                        | OPTION_MASK_ABI_X32
   1257                                                        | OPTION_MASK_CODE16);
   1258                    opts->x_ix86_isa_flags2 = 0;
   1259                    opts->x_ix86_isa_flags2_explicit = 0;
   1260                  }

That seems logical because target attribute is used for e.g. ifunc multi-versioning and one needs
to be sure all existing ISA flags are dropped. However, I noticed clang behaves differently:

$ cat hreset.c
#pragma GCC target "arch=geode"
#include <immintrin.h>
void foo(unsigned int eax)
{
   _hreset (eax);
}

$ clang hreset.c -mhreset  -c -O2 -m32
$ gcc hreset.c -mhreset  -c -O2 -m32
In file included from /home/marxin/bin/gcc/lib64/gcc/x86_64-pc-linux-gnu/12.0.0/include/x86gprintrin.h:97,
                  from /home/marxin/bin/gcc/lib64/gcc/x86_64-pc-linux-gnu/12.0.0/include/immintrin.h:27,
                  from hreset.c:2:
hreset.c: In function ‘foo’:
/home/marxin/bin/gcc/lib64/gcc/x86_64-pc-linux-gnu/12.0.0/include/hresetintrin.h:39:1: error: inlining failed in call to ‘always_inline’ ‘_hreset’: target specific option mismatch
    39 | _hreset (unsigned int __EAX)
       | ^~~~~~~
hreset.c:5:3: note: called from here
     5 |   _hreset (eax);
       |   ^~~~~~~~~~~~~

Anyway, I think the current target attribute handling should be preserved.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

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