[PATCH] rs6000: Fix restored rs6000_long_double_type_size.

[PATCH] rs6000: Fix restored rs6000_long_double_type_size.

[Martin Liška]

Hello.

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.

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

Ready to be installed?
Thanks,
Martin

gcc/ChangeLog:

	* config/rs6000/rs6000.c (rs6000_option_override_internal): When
	a target option is restored, it can have
	rs6000_long_double_type_size set to FLOAT_PRECISION_TFmode.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/pragma-optimize.c: New test.
---
  gcc/config/rs6000/rs6000.c                         |  2 ++
  gcc/testsuite/gcc.target/powerpc/pragma-optimize.c | 14 ++++++++++++++
  2 files changed, 16 insertions(+)
  create mode 100644 gcc/testsuite/gcc.target/powerpc/pragma-optimize.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 835af7708f9..2c811480db9 100644
--- 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.  */
    else if (rs6000_long_double_type_size == 128)
      rs6000_long_double_type_size = FLOAT_PRECISION_TFmode;
    else if (global_options_set.x_rs6000_ieeequad)
diff --git a/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
new file mode 100644
index 00000000000..629bfcee0ec
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
@@ -0,0 +1,14 @@
+/* { dg-do compile { target { powerpc*-*-linux* } } } */
+/* { 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) *
+f_ifunc (void)
+{
+  __typeof (f) *res = x ? f_power9 : f_power8;
+  return res;
+}
-- 
2.32.0

Re: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.

[Segher Boessenkool]

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?

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

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

> +/* { 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.

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

Re: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 2147 bytes --]

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
> 


[-- Attachment #2: 0001-rs6000-Fix-restored-rs6000_long_double_type_size.patch --]
[-- Type: text/x-patch, Size: 2070 bytes --]

From 1632939853fbf193f72ace3d1024a137d549fef4 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Tue, 1 Jun 2021 15:39:14 +0200
Subject: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.

gcc/ChangeLog:

	* config/rs6000/rs6000.c (rs6000_option_override_internal): When
	a target option is restored, it can have
	rs6000_long_double_type_size set to FLOAT_PRECISION_TFmode.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/pragma-optimize.c: New test.
---
 gcc/config/rs6000/rs6000.c                         |  2 ++
 gcc/testsuite/gcc.target/powerpc/pragma-optimize.c | 14 ++++++++++++++
 2 files changed, 16 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pragma-optimize.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 2c249e186e1..fa4aa864c00 100644
--- 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 with cl_target_option_restore.  */
   else if (rs6000_long_double_type_size == 128)
     rs6000_long_double_type_size = FLOAT_PRECISION_TFmode;
   else if (global_options_set.x_rs6000_ieeequad)
diff --git a/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
new file mode 100644
index 00000000000..2455fb57138
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
@@ -0,0 +1,14 @@
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { 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) *
+f_ifunc (void)
+{
+  __typeof (f) *res = x ? f_power9 : f_power8;
+  return res;
+}
-- 
2.32.0

Re: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.

[Martin Liška]

PING^1

On 6/28/21 2:19 PM, Martin Liška wrote:
> 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
>>
> 

Re: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.

[Segher Boessenkool]

On Mon, Jul 12, 2021 at 06:19:28AM +0200, Martin Liška wrote:
> PING^1

I did not notice you attached a new patch.  It works a lot better if
every patch series is a new thread.


Segher

Re: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.

[Segher Boessenkool]

On Mon, Jun 28, 2021 at 02:19:03PM +0200, Martin Liška wrote:
> On 6/24/21 12:46 AM, Segher Boessenkool 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.


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

Ugh.  Yes, the status quo is no good either :-(

> >>+/* { 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.

So it behaves differently if you select the default than if you do not
select anything?  That is wrong, no?

> >From 1632939853fbf193f72ace3d1024a137d549fef4 Mon Sep 17 00:00:00 2001
> From: Martin Liska <mliska@suse.cz>
> Date: Tue, 1 Jun 2021 15:39:14 +0200
> Subject: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.

(No full stop at end of subject please)

Missing patch description here.  This should be suitable as commit
message when you eventually commit the patch.

Please send with that, as a separate mail, not as attachment to another
thread.

> gcc/ChangeLog:
> 
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): When
> 	a target option is restored, it can have
> 	rs6000_long_double_type_size set to FLOAT_PRECISION_TFmode.

That does not say what changed?

> --- 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 with cl_target_option_restore.  */
>    else if (rs6000_long_double_type_size == 128)
>      rs6000_long_double_type_size = FLOAT_PRECISION_TFmode;
>    else if (global_options_set.x_rs6000_ieeequad)

"The option can be restored" is more confusing than helpful.  *Will* be
restored by it, maybe?  Not that I understand what that means :-/

Does it make more sense to merge the 128 and FLOAT_PRECISION_TFmode
cases?

> diff --git a/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
> new file mode 100644
> index 00000000000..2455fb57138
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile { target { powerpc*-*-* } } } */

No target powerpc*-*-* in gcc.target/powerpc please.  This is enforced
for everything in there by powerpc.exp already.

Thanks,


Segher

[PATCH] rs6000: Fix restored rs6000_long_double_type_size

[Martin Liska]

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.

gcc/ChangeLog:

	* config/rs6000/rs6000.c (rs6000_option_override_internal): When
	a target option is restored, it can have
	rs6000_long_double_type_size set to FLOAT_PRECISION_TFmode
	and error should not be emitted.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/pragma-optimize.c: New test.
---
 gcc/config/rs6000/rs6000.c                         |  2 ++
 gcc/testsuite/gcc.target/powerpc/pragma-optimize.c | 13 +++++++++++++
 2 files changed, 15 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pragma-optimize.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 279f00cc648..510936a9948 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -4186,6 +4186,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 value can be seen when cl_target_option_restore is called.  */
   else if (rs6000_long_double_type_size == 128)
     rs6000_long_double_type_size = FLOAT_PRECISION_TFmode;
   else if (global_options_set.x_rs6000_ieeequad)
diff --git a/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
new file mode 100644
index 00000000000..4a86b58f27c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
@@ -0,0 +1,13 @@
+/* { 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) *
+f_ifunc (void)
+{
+  __typeof (f) *res = x ? f_power9 : f_power8;
+  return res;
+}
-- 
2.32.0

[PATCH] rs6000: Fix restored rs6000_long_double_type_size

[Martin Liska]

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.

gcc/ChangeLog:

	* config/rs6000/rs6000.c (rs6000_option_override_internal): When
	a target option is restored, it can have
	rs6000_long_double_type_size set to FLOAT_PRECISION_TFmode
	and error should not be emitted.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/pragma-optimize.c: New test.
---
 gcc/config/rs6000/rs6000.c                         |  2 ++
 gcc/testsuite/gcc.target/powerpc/pragma-optimize.c | 13 +++++++++++++
 2 files changed, 15 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pragma-optimize.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 279f00cc648..510936a9948 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -4186,6 +4186,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 value can be seen when cl_target_option_restore is called.  */
   else if (rs6000_long_double_type_size == 128)
     rs6000_long_double_type_size = FLOAT_PRECISION_TFmode;
   else if (global_options_set.x_rs6000_ieeequad)
diff --git a/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
new file mode 100644
index 00000000000..4a86b58f27c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
@@ -0,0 +1,13 @@
+/* { 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) *
+f_ifunc (void)
+{
+  __typeof (f) *res = x ? f_power9 : f_power8;
+  return res;
+}
-- 
2.32.0

[PATCH] rs6000: Fix restored rs6000_long_double_type_size

[Martin Liska]

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.

gcc/ChangeLog:

	* config/rs6000/rs6000.c (rs6000_option_override_internal): When
	a target option is restored, it can have
	rs6000_long_double_type_size set to FLOAT_PRECISION_TFmode
	and error should not be emitted.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/pragma-optimize.c: New test.
---
 gcc/config/rs6000/rs6000.c                         |  2 ++
 gcc/testsuite/gcc.target/powerpc/pragma-optimize.c | 13 +++++++++++++
 2 files changed, 15 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pragma-optimize.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 279f00cc648..510936a9948 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -4186,6 +4186,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 value can be seen when cl_target_option_restore is called.  */
   else if (rs6000_long_double_type_size == 128)
     rs6000_long_double_type_size = FLOAT_PRECISION_TFmode;
   else if (global_options_set.x_rs6000_ieeequad)
diff --git a/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
new file mode 100644
index 00000000000..4a86b58f27c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
@@ -0,0 +1,13 @@
+/* { 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) *
+f_ifunc (void)
+{
+  __typeof (f) *res = x ? f_power9 : f_power8;
+  return res;
+}
-- 
2.32.0

Re: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.

[Martin Liška]

On 7/12/21 7:20 PM, Segher Boessenkool wrote:
> On Mon, Jun 28, 2021 at 02:19:03PM +0200, Martin Liška wrote:
>> On 6/24/21 12:46 AM, Segher Boessenkool 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.
> 
> 
>>>> --- /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.
> 
> Ugh.  Yes, the status quo is no good either :-(
> 
>>>> +/* { 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.
> 
> So it behaves differently if you select the default than if you do not
> select anything?  That is wrong, no?

Sorry, I don't get your example, please explain it.

> 
>> >From 1632939853fbf193f72ace3d1024a137d549fef4 Mon Sep 17 00:00:00 2001
>> From: Martin Liska <mliska@suse.cz>
>> Date: Tue, 1 Jun 2021 15:39:14 +0200
>> Subject: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.
> 
> (No full stop at end of subject please)

Done.

> 
> Missing patch description here.  This should be suitable as commit
> message when you eventually commit the patch.
> 
> Please send with that, as a separate mail, not as attachment to another
> thread.

Done.

> 
>> gcc/ChangeLog:
>>
>> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): When
>> 	a target option is restored, it can have
>> 	rs6000_long_double_type_size set to FLOAT_PRECISION_TFmode.
> 
> That does not say what changed?

Updated.

> 
>> --- 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 with cl_target_option_restore.  */
>>     else if (rs6000_long_double_type_size == 128)
>>       rs6000_long_double_type_size = FLOAT_PRECISION_TFmode;
>>     else if (global_options_set.x_rs6000_ieeequad)
> 
> "The option can be restored" is more confusing than helpful.  *Will* be
> restored by it, maybe?  Not that I understand what that means :-/

I updated the wording a bit.

> 
> Does it make more sense to merge the 128 and FLOAT_PRECISION_TFmode
> cases?

Likely not, it's important to assign a reasonable comment to the 128 option value.

> 
>> diff --git a/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
>> new file mode 100644
>> index 00000000000..2455fb57138
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
>> @@ -0,0 +1,14 @@
>> +/* { dg-do compile { target { powerpc*-*-* } } } */
> 
> No target powerpc*-*-* in gcc.target/powerpc please.  This is enforced
> for everything in there by powerpc.exp already.

Fixed.

Martin

> 
> Thanks,
> 
> 
> Segher
> 

Re: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.

[Segher Boessenkool]

Hi!

On Fri, Jul 23, 2021 at 07:47:54AM +0200, Martin Liška wrote:
> On 7/12/21 7:20 PM, Segher Boessenkool wrote:
> >>>>+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.
> >
> >So it behaves differently if you select the default than if you do not
> >select anything?  That is wrong, no?
> 
> Sorry, I don't get your example, please explain it.

If -mbork is the default, the coompiler whould behave the same if you
invoke it with -mbork as when you do not.  And the optimize attribute
should work exactly the same as command line options.

Or perhaps you are saying you have this in the testcase only to exercise
the option save/restore code paths?  Please document that then, in the
testcase.


Segher

Re: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

On 7/23/21 7:57 PM, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, Jul 23, 2021 at 07:47:54AM +0200, Martin Liška wrote:
>> On 7/12/21 7:20 PM, Segher Boessenkool wrote:
>>>>>> +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.
>>>
>>> So it behaves differently if you select the default than if you do not
>>> select anything?  That is wrong, no?
>>
>> Sorry, I don't get your example, please explain it.
> 
> If -mbork is the default, the coompiler whould behave the same if you
> invoke it with -mbork as when you do not.  And the optimize attribute
> should work exactly the same as command line options.

Ah, got your point. All right, let's use then 'optimize(1)'.

Is it fine with the adjustment?
Cheers,
Martin

> 
> Or perhaps you are saying you have this in the testcase only to exercise
> the option save/restore code paths?  Please document that then, in the
> testcase.
> 
> 
> Segher
> 


[-- Attachment #2: 0001-rs6000-Fix-restored-rs6000_long_double_type_size.patch --]
[-- Type: text/x-patch, Size: 2236 bytes --]

From 517e32a75aa59b4b538eda30e78de0fa925bb0f9 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Tue, 1 Jun 2021 15:39:14 +0200
Subject: [PATCH] rs6000: Fix restored rs6000_long_double_type_size

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.

gcc/ChangeLog:

	* config/rs6000/rs6000.c (rs6000_option_override_internal): When
	a target option is restored, it can have
	rs6000_long_double_type_size set to FLOAT_PRECISION_TFmode
	and error should not be emitted.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/pragma-optimize.c: New test.
---
 gcc/config/rs6000/rs6000.c                         |  2 ++
 gcc/testsuite/gcc.target/powerpc/pragma-optimize.c | 13 +++++++++++++
 2 files changed, 15 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pragma-optimize.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 2de5a96e1b6..5b1c06b09fc 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -4189,6 +4189,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 value can be seen when cl_target_option_restore is called.  */
   else if (rs6000_long_double_type_size == 128)
     rs6000_long_double_type_size = FLOAT_PRECISION_TFmode;
   else if (global_options_set.x_rs6000_ieeequad)
diff --git a/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
new file mode 100644
index 00000000000..e8ba63a0667
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pragma-optimize.c
@@ -0,0 +1,13 @@
+/* { 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 (1))) __typeof (f) *
+f_ifunc (void)
+{
+  __typeof (f) *res = x ? f_power9 : f_power8;
+  return res;
+}
-- 
2.32.0

Re: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.

[Segher Boessenkool]

On Thu, Aug 05, 2021 at 02:05:24PM +0200, Martin Liška wrote:
> On 7/23/21 7:57 PM, Segher Boessenkool wrote:
> >Hi!
> >
> >On Fri, Jul 23, 2021 at 07:47:54AM +0200, Martin Liška wrote:
> >>On 7/12/21 7:20 PM, Segher Boessenkool wrote:
> >>>>>>+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.
> >>>
> >>>So it behaves differently if you select the default than if you do not
> >>>select anything?  That is wrong, no?
> >>
> >>Sorry, I don't get your example, please explain it.
> >
> >If -mbork is the default, the coompiler whould behave the same if you
> >invoke it with -mbork as when you do not.  And the optimize attribute
> >should work exactly the same as command line options.
> 
> Ah, got your point. All right, let's use then 'optimize(1)'.
> 
> Is it fine with the adjustment?

You are saying the compiler's behaviour is broken, but are changing the
testcase to avoid exhibiting that behaviour?  No, this is not fine.

If a flag is the default the compiler should do the same thing with and
without any attribute setting that flag, directly or indirectly.


Segher

Re: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.

[Martin Liška]

On 8/5/21 5:39 PM, Segher Boessenkool wrote:
> On Thu, Aug 05, 2021 at 02:05:24PM +0200, Martin Liška wrote:
>> On 7/23/21 7:57 PM, Segher Boessenkool wrote:
>>> Hi!
>>>
>>> On Fri, Jul 23, 2021 at 07:47:54AM +0200, Martin Liška wrote:
>>>> On 7/12/21 7:20 PM, Segher Boessenkool wrote:
>>>>>>>> +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.
>>>>>
>>>>> So it behaves differently if you select the default than if you do not
>>>>> select anything?  That is wrong, no?
>>>>
>>>> Sorry, I don't get your example, please explain it.
>>>
>>> If -mbork is the default, the coompiler whould behave the same if you
>>> invoke it with -mbork as when you do not.  And the optimize attribute
>>> should work exactly the same as command line options.
>>
>> Ah, got your point. All right, let's use then 'optimize(1)'.
>>
>> Is it fine with the adjustment?
> 
> You are saying the compiler's behaviour is broken, but are changing the
> testcase to avoid exhibiting that behaviour?

No, both selections of the 'optimize' attribute trigger the ICE. The reason
is that any optimize attribute triggers eventually cl_target_option_save/restore
mechanism which is what the patch addresses.

> No, this is not fine.
> 
> If a flag is the default the compiler should do the same thing with and
> without any attribute setting that flag, directly or indirectly.

Yes, but should not crash. And that's what is my patch dealing with.

Cheers,
Martin

> 
> 
> Segher
> 

Re: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.

[Segher Boessenkool]

On Thu, Aug 05, 2021 at 06:49:21PM +0200, Martin Liška wrote:
> On 8/5/21 5:39 PM, Segher Boessenkool wrote:
> >>>If -mbork is the default, the coompiler whould behave the same if you
> >>>invoke it with -mbork as when you do not.  And the optimize attribute
> >>>should work exactly the same as command line options.
> >>
> >>Ah, got your point. All right, let's use then 'optimize(1)'.
> >>
> >>Is it fine with the adjustment?
> >
> >You are saying the compiler's behaviour is broken, but are changing the
> >testcase to avoid exhibiting that behaviour?
> 
> No, both selections of the 'optimize' attribute trigger the ICE. The reason
> is that any optimize attribute triggers eventually 
> cl_target_option_save/restore
> mechanism which is what the patch addresses.

Aha.  So say that in the test case!  That the test case is to check if
the bug fixed in XXXXXXXXXXXX has been reintroduced (or describe the bug
more precisely if needed).

Okay for trunk with that fixed.  Thanks!


Segher

Re: [PATCH] rs6000: Fix restored rs6000_long_double_type_size

[Segher Boessenkool]

Hi!

On Tue, Jun 01, 2021 at 03:39:14PM +0200, Martin Liska wrote:
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): When
> 	a target option is restored, it can have
> 	rs6000_long_double_type_size set to FLOAT_PRECISION_TFmode
> 	and error should not be emitted.

> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -4186,6 +4186,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 value can be seen when cl_target_option_restore is called.  */

(line too long)

The comment is still very cryptic.  What you have in the changelog is
much better digestible.

Okay for trunk with those things fixed.  Thanks!


Segher