* RFA: PR 70044: Catch a second call to aarch64_override_options_after_change
@ 2016-03-04 13:44 Nick Clifton
2016-03-04 14:16 ` Kyrill Tkachov
0 siblings, 1 reply; 6+ messages in thread
From: Nick Clifton @ 2016-03-04 13:44 UTC (permalink / raw)
To: marcus.shawcroft, richard.earnshaw; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]
Hi Markus, Hi Richard,
PR 70044 reports a problem with the AArch64 backend. With LTO enabled
the function aarch64_override_options_after_change can be called more
than once for the same function. If only leaf frame pointers were
being omitted originally, then the first call will set
flag_omit_frame_pointer to true. Then the second call will set
flag_omit_leaf_frame_pointer to false, thus forcing the frame pointer
to always be omitted, contrary to the original settings of these
flags.
The attached patch fixes this problem by setting
flag_omit_frame_pointer to true, but using a special value of 2 to do
so. Then when the second call occurs we can detect this case and
ensure that we do not set flag_omit_leaf_frame_pointer to false.
Tested with no regressions on an aarch64-elf toolchain.
OK to apply ?
Cheers
Nick
gcc/ChangeLog
2016-03-04 Nick Clifton <nickc@redhat.com>
PR target/7044
* config/aarch64/aarch64.c
(aarch64_override_options_after_change_1): When forcing
flag_omit_frame_pointer to be true, use a special value that can
be detected if this function is called again, thus preventing
flag_omit_leaf_frame_pointer from being forced to be false.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: aarch64.c.patch --]
[-- Type: text/x-patch, Size: 1326 bytes --]
Index: gcc/config/aarch64/aarch64.c
===================================================================
--- gcc/config/aarch64/aarch64.c (revision 233960)
+++ gcc/config/aarch64/aarch64.c (working copy)
@@ -8110,6 +8110,21 @@
static void
aarch64_override_options_after_change_1 (struct gcc_options *opts)
{
+ /* The logic here is that if we are disabling all frame pointer generation
+ then we do not need to disable leaf frame pointer generation as a
+ separate operation. But if we are *only* disabling leaf frame pointer
+ generation then we set flag_omit_frame_pointer to true, but in
+ aarch64_frame_pointer_required we return false only for leaf functions.
+
+ PR 70044: We have to be careful about being called multiple times for the
+ same function. Once we have decided to set flag_omit_frame_pointer just
+ so that we can omit leaf frame pointers, we must then not interpret a
+ second call as meaning that all frame pointer generation should be
+ omitted. We do this by setting flag_omit_frame_pointer to a special,
+ non-zero value. */
+ if (opts->x_flag_omit_frame_pointer == 2)
+ opts->x_flag_omit_frame_pointer = 0;
+
if (opts->x_flag_omit_frame_pointer)
opts->x_flag_omit_leaf_frame_pointer = false;
else if (opts->x_flag_omit_leaf_frame_pointer)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFA: PR 70044: Catch a second call to aarch64_override_options_after_change
2016-03-04 13:44 RFA: PR 70044: Catch a second call to aarch64_override_options_after_change Nick Clifton
@ 2016-03-04 14:16 ` Kyrill Tkachov
2016-03-07 13:12 ` Nick Clifton
0 siblings, 1 reply; 6+ messages in thread
From: Kyrill Tkachov @ 2016-03-04 14:16 UTC (permalink / raw)
To: Nick Clifton, marcus.shawcroft, richard.earnshaw; +Cc: gcc-patches
Hi Nick,
On 04/03/16 13:44, Nick Clifton wrote:
> Hi Markus, Hi Richard,
>
> PR 70044 reports a problem with the AArch64 backend. With LTO enabled
> the function aarch64_override_options_after_change can be called more
> than once for the same function. If only leaf frame pointers were
> being omitted originally, then the first call will set
> flag_omit_frame_pointer to true. Then the second call will set
> flag_omit_leaf_frame_pointer to false, thus forcing the frame pointer
> to always be omitted, contrary to the original settings of these
> flags.
>
> The attached patch fixes this problem by setting
> flag_omit_frame_pointer to true, but using a special value of 2 to do
> so. Then when the second call occurs we can detect this case and
> ensure that we do not set flag_omit_leaf_frame_pointer to false.
>
> Tested with no regressions on an aarch64-elf toolchain.
>
> OK to apply ?
Thanks for looking at this.
> Cheers
> Nick
>
> gcc/ChangeLog
> 2016-03-04 Nick Clifton <nickc@redhat.com>
>
> PR target/7044
> * config/aarch64/aarch64.c
> (aarch64_override_options_after_change_1): When forcing
> flag_omit_frame_pointer to be true, use a special value that can
> be detected if this function is called again, thus preventing
> flag_omit_leaf_frame_pointer from being forced to be false.
>
+ /* The logic here is that if we are disabling all frame pointer generation
+ then we do not need to disable leaf frame pointer generation as a
+ separate operation. But if we are*only* disabling leaf frame pointer
+ generation then we set flag_omit_frame_pointer to true, but in
+ aarch64_frame_pointer_required we return false only for leaf functions.
+
+ PR 70044: We have to be careful about being called multiple times for the
+ same function. Once we have decided to set flag_omit_frame_pointer just
+ so that we can omit leaf frame pointers, we must then not interpret a
+ second call as meaning that all frame pointer generation should be
+ omitted. We do this by setting flag_omit_frame_pointer to a special,
+ non-zero value. */
+ if (opts->x_flag_omit_frame_pointer == 2)
+ opts->x_flag_omit_frame_pointer = 0;
+
if (opts->x_flag_omit_frame_pointer)
opts->x_flag_omit_leaf_frame_pointer = false;
else if (opts->x_flag_omit_leaf_frame_pointer)
This is missing a second hunk from the patch you attached in the PR that I think is necessary
for this to work (setting to x_flag_omit_frame_pointer)...
I've been testing a very similar patch that just changes that logic to:
if (opts->x_flag_omit_frame_pointer == 1)
opts->x_flag_omit_leaf_frame_pointer = 0;
else if (opts->x_flag_omit_leaf_frame_pointer)
opts->x_flag_omit_frame_pointer = 2;
Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
That's my bad and I'll propose a patch for it soon.
Also, is there a way to create a testcase for the testuite?
i.e. is there a simple way to scan the assembly generated after the final LTO processing
for the presence of the frame pointer?
Thanks,
Kyrill
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFA: PR 70044: Catch a second call to aarch64_override_options_after_change
2016-03-04 14:16 ` Kyrill Tkachov
@ 2016-03-07 13:12 ` Nick Clifton
2016-03-10 15:23 ` James Greenhalgh
0 siblings, 1 reply; 6+ messages in thread
From: Nick Clifton @ 2016-03-07 13:12 UTC (permalink / raw)
To: Kyrill Tkachov, marcus.shawcroft, richard.earnshaw; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]
Hi Kyrill,
> This is missing a second hunk from the patch you attached in the PR that I think is necessary
> for this to work (setting to x_flag_omit_frame_pointer)...
Doh! Silly me - there was a snafu restoring the patch after I had reverted it in order to
check that the pre- and post- patch gcc test results were the same.
> Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
> where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
> That's my bad and I'll propose a patch for it soon.
OK.
> Also, is there a way to create a testcase for the testuite?
> i.e. is there a simple way to scan the assembly generated after the final LTO processing
> for the presence of the frame pointer?
Originally I thought not. But then I found scan-lto-assembler in testsuite/lib/scanasm.exp
and that made everything simple.
So attached is a revised patch with the missing second hunk restored and a testcase added.
(Which I have checked and confirmed that it does fail without the patch and it does pass
with the patch applied).
OK to apply ?
Cheers
Nick
gcc/ChangeLog as before...
gcc/testsuite/ChangeLog
2016-03-07 Nick Clifton <nickc@redhat.com>
PR target/70044
* gcc.target/aarch64/pr70044.c: New test.
[-- Attachment #2: aarch64.c.patch.2 --]
[-- Type: application/x-troff-man, Size: 2021 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFA: PR 70044: Catch a second call to aarch64_override_options_after_change
2016-03-07 13:12 ` Nick Clifton
@ 2016-03-10 15:23 ` James Greenhalgh
2016-03-10 15:43 ` Kyrill Tkachov
2016-03-10 17:27 ` Nick Clifton
0 siblings, 2 replies; 6+ messages in thread
From: James Greenhalgh @ 2016-03-10 15:23 UTC (permalink / raw)
To: Nick Clifton
Cc: Kyrill Tkachov, marcus.shawcroft, richard.earnshaw, gcc-patches
On Mon, Mar 07, 2016 at 01:12:16PM +0000, Nick Clifton wrote:
> Hi Kyrill,
>
> > This is missing a second hunk from the patch you attached in the PR that I think is necessary
> > for this to work (setting to x_flag_omit_frame_pointer)...
>
> Doh! Silly me - there was a snafu restoring the patch after I had reverted it in order to
> check that the pre- and post- patch gcc test results were the same.
>
> > Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
> > where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
> > That's my bad and I'll propose a patch for it soon.
>
> OK.
>
> > Also, is there a way to create a testcase for the testuite?
> > i.e. is there a simple way to scan the assembly generated after the final LTO processing
> > for the presence of the frame pointer?
>
> Originally I thought not. But then I found scan-lto-assembler in testsuite/lib/scanasm.exp
> and that made everything simple.
>
> So attached is a revised patch with the missing second hunk restored and a testcase added.
> (Which I have checked and confirmed that it does fail without the patch and it does pass
> with the patch applied).
>
> OK to apply ?
OK, thanks.
> > Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
> > where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
> > That's my bad and I'll propose a patch for it soon.
I don't think I've seen this on list yet, it might be worth waiting until
Kyrill has put this patch up before you commit.
Thanks,
James
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFA: PR 70044: Catch a second call to aarch64_override_options_after_change
2016-03-10 15:23 ` James Greenhalgh
@ 2016-03-10 15:43 ` Kyrill Tkachov
2016-03-10 17:27 ` Nick Clifton
1 sibling, 0 replies; 6+ messages in thread
From: Kyrill Tkachov @ 2016-03-10 15:43 UTC (permalink / raw)
To: James Greenhalgh, Nick Clifton
Cc: marcus.shawcroft, richard.earnshaw, gcc-patches
On 10/03/16 15:23, James Greenhalgh wrote:
> On Mon, Mar 07, 2016 at 01:12:16PM +0000, Nick Clifton wrote:
>> Hi Kyrill,
>>
>>> This is missing a second hunk from the patch you attached in the PR that I think is necessary
>>> for this to work (setting to x_flag_omit_frame_pointer)...
>> Doh! Silly me - there was a snafu restoring the patch after I had reverted it in order to
>> check that the pre- and post- patch gcc test results were the same.
>>
>>> Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
>>> where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
>>> That's my bad and I'll propose a patch for it soon.
>> OK.
>>
>>> Also, is there a way to create a testcase for the testuite?
>>> i.e. is there a simple way to scan the assembly generated after the final LTO processing
>>> for the presence of the frame pointer?
>> Originally I thought not. But then I found scan-lto-assembler in testsuite/lib/scanasm.exp
>> and that made everything simple.
>>
>> So attached is a revised patch with the missing second hunk restored and a testcase added.
>> (Which I have checked and confirmed that it does fail without the patch and it does pass
>> with the patch applied).
>>
>> OK to apply ?
> OK, thanks.
>
>>> Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
>>> where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
>>> That's my bad and I'll propose a patch for it soon.
> I don't think I've seen this on list yet, it might be worth waiting until
> Kyrill has put this patch up before you commit.
Posted at https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00638.html
Kyrill
> Thanks,
> James
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: RFA: PR 70044: Catch a second call to aarch64_override_options_after_change
2016-03-10 15:23 ` James Greenhalgh
2016-03-10 15:43 ` Kyrill Tkachov
@ 2016-03-10 17:27 ` Nick Clifton
1 sibling, 0 replies; 6+ messages in thread
From: Nick Clifton @ 2016-03-10 17:27 UTC (permalink / raw)
To: James Greenhalgh
Cc: Kyrill Tkachov, marcus.shawcroft, richard.earnshaw, gcc-patches
Hi James,
>> OK to apply ?
>
> OK, thanks.
Thanks - applied.
>>> Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
>>> where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
>>> That's my bad and I'll propose a patch for it soon.
>
> I don't think I've seen this on list yet, it might be worth waiting until
> Kyrill has put this patch up before you commit.
I did this. Plus I checked to make sure that the patch still works and that the new test passes...
Cheers
Nick
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-03-10 17:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-04 13:44 RFA: PR 70044: Catch a second call to aarch64_override_options_after_change Nick Clifton
2016-03-04 14:16 ` Kyrill Tkachov
2016-03-07 13:12 ` Nick Clifton
2016-03-10 15:23 ` James Greenhalgh
2016-03-10 15:43 ` Kyrill Tkachov
2016-03-10 17:27 ` Nick Clifton
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).