On 6/24/21 12:46 AM, Segher Boessenkool wrote:
> Hi!
> 
> On Wed, Jun 23, 2021 at 03:22:34PM +0200, Martin Liška wrote:
>> As mentioned in the "Fallout: save/restore target options in
>> handle_optimize_attribute"
>> thread, we need to support target option restore of
>> rs6000_long_double_type_size == FLOAT_PRECISION_TFmode.
> 
> I have no idea?  Could you explain please?

Sure. Few weeks ago, we started using cl_target_option_{save,restore} calls
even for optimize attributes (and pragma). Motivation was that optimize options
can influence target options (and vice versa).

Doing that, FLOAT_PRECISION_TFmode must be accepted as a valid option value
for rs6000_long_double_type_size.

> 
>> --- a/gcc/config/rs6000/rs6000.c
>> +++ b/gcc/config/rs6000/rs6000.c
>> @@ -4185,6 +4185,8 @@ rs6000_option_override_internal (bool global_init_p)
>>         else
>>   	rs6000_long_double_type_size = default_long_double_size;
>>       }
>> +  else if (rs6000_long_double_type_size == FLOAT_PRECISION_TFmode)
>> +    ; /* The option can be restored a TREE_TARGET_OPTION.  */
> 
> What does that mean?  It is not grammatical, and not obvious what it
> should mean.

Updated.

> 
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
>> @@ -0,0 +1,14 @@
>> +/* { dg-do compile { target { powerpc*-*-linux* } } } */
> 
> Why on Linux only?  That doesn't sound right.  Do you need some other
> selector(s)?

Sorry, I copied the test-case.

> 
>> +/* { dg-options "-O2 -mlong-double-128 -mabi=ibmlongdouble" } */
>> +
>> +extern unsigned long int x;
>> +extern float f (float);
>> +extern __typeof (f) f_power8;
>> +extern __typeof (f) f_power9;
>> +extern __typeof (f) f __attribute__ ((ifunc ("f_ifunc")));
>> +static __attribute__ ((optimize ("-fno-stack-protector"))) __typeof (f) *
> 
> -fno-stack-protector is default.

Yes, but one needs an optimize attribute in order to trigger cl_target_option_save/restore
mechanism.

Martin

> 
>> +f_ifunc (void)
>> +{
>> +  __typeof (f) *res = x ? f_power9 : f_power8;
>> +  return res;
>> +}
> 
> The testcase should say what it is testing for, it is not obvious?
> 
> 
> Segher
>