From: "Martin Liška" <mliska@suse.cz>
To: Segher Boessenkool <segher@kernel.crashing.org>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] rs6000: Fix restored rs6000_long_double_type_size.
Date: Fri, 23 Jul 2021 07:47:54 +0200 [thread overview]
Message-ID: <cdd6af83-49fc-1054-647e-ccabeafd4efa@suse.cz> (raw)
In-Reply-To: <20210712172048.GC1583@gate.crashing.org>
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
>
next prev parent reply other threads:[~2021-07-23 5:47 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-23 13:22 Martin Liška
2021-06-23 22:46 ` Segher Boessenkool
2021-06-28 12:19 ` Martin Liška
2021-07-12 4:19 ` Martin Liška
2021-07-12 17:00 ` Segher Boessenkool
2021-07-12 17:20 ` Segher Boessenkool
2021-07-23 5:45 ` Martin Liska
2021-07-23 5:45 ` Martin Liska
2021-07-23 5:45 ` Martin Liska
2021-07-23 5:47 ` Martin Liška [this message]
2021-07-23 17:57 ` Segher Boessenkool
2021-08-05 12:05 ` Martin Liška
2021-08-05 15:39 ` Segher Boessenkool
2021-08-05 16:49 ` Martin Liška
2021-08-05 20:06 ` Segher Boessenkool
[not found] ` <202107230545.16N5jkeY006982@gate.crashing.org>
2021-07-23 18:00 ` Segher Boessenkool
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cdd6af83-49fc-1054-647e-ccabeafd4efa@suse.cz \
--to=mliska@suse.cz \
--cc=gcc-patches@gcc.gnu.org \
--cc=segher@kernel.crashing.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).