[PATCH] Optimize macro: make it more predictable

Martin Liška mliska@suse.cz
Tue Aug 10 15:52:52 GMT 2021


PING^1

On 7/1/21 3:13 PM, Martin Liška wrote:
> 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
> 



More information about the Gcc-patches mailing list