[ARM][AArch64] Default to -fno-common

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[ARM][AArch64] Default to -fno-common

suyash singh via cfe-dev
Hello,

Subject says most of it: I would like to propose that the Arm targets default to -fno-common for performance and code-size reasons. In addition, GCC has also flipped the switch recently, see [1], so we would get the same behaviour as GCC which would also be nice. I've learned that the switch for GCC was relatively straightforward (for users), and there is some GNU documentation with Q&As to help toolchain users to transition if they find problems. I have put a draft patch up for review here: https://reviews.llvm.org/D75056. This definitely needs some doc changes (and I need to look at the tests), but first I would like to see how we feel about flipping the switch: any ideas, comments, objections?

Cheers,
Sjoerd.


[1] The GCC commit:

commit 6271dd984d7f920d4fb17ad37af6a1f8e6b796dc
Author: Wilco Dijkstra <[hidden email]>
Date:   Wed Nov 20 16:29:23 2019 +0000

    PR85678: Change default to -fno-common
    GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
    C feature which is not conforming with the latest C standards.  On many targets
    this means global variable accesses have a codesize and performance penalty.
    This applies to C code only, C++ code is not affected by -fcommon.  It is about
    time to change the default.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [ARM][AArch64] Default to -fno-common

suyash singh via cfe-dev
On 02/24/2020 08:28 AM, Sjoerd Meijer via cfe-dev wrote:
> Hello,
>
> Subject says most of it: I would like to propose that the Arm targets default to -fno-common for performance and code-size reasons. In addition, GCC has also flipped the switch recently, see [1], so we would get the same behaviour as GCC which would also be nice. I've learned that the switch for GCC was relatively straightforward (for users), and there is some GNU documentation with Q&As to help toolchain users to transition if they find problems. I have put a draft patch up for review here: https://reviews.llvm.org/D75056. This definitely needs some doc changes (and I need to look at the tests), but first I would like to see how we feel about flipping the switch: any ideas, comments, objections?
>

Is there any reason to limit this to just the ARM targets?

-Tom

> Cheers,
> Sjoerd.
>
>
> [1] The GCC commit:
>
> commit 6271dd984d7f920d4fb17ad37af6a1f8e6b796dc
> Author: Wilco Dijkstra <[hidden email]>
> Date:   Wed Nov 20 16:29:23 2019 +0000
>
>     PR85678: Change default to -fno-common
>     GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
>     C feature which is not conforming with the latest C standards.  On many targets
>     this means global variable accesses have a codesize and performance penalty.
>     This applies to C code only, C++ code is not affected by -fcommon.  It is about
>     time to change the default.
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [ARM][AArch64] Default to -fno-common

suyash singh via cfe-dev
>  Is there any reason to limit this to just the ARM targets?

No good reason other than that I don't know how beneficial this is for other targets.
But I definitely don't mind giving other targets the same treatment though while I am at it.

Cheers,
Sjoerd.


From: Tom Stellard <[hidden email]>
Sent: 24 February 2020 16:31
To: Sjoerd Meijer <[hidden email]>; [hidden email] Developers <[hidden email]>
Subject: Re: [cfe-dev] [ARM][AArch64] Default to -fno-common
 
On 02/24/2020 08:28 AM, Sjoerd Meijer via cfe-dev wrote:
> Hello,
>
> Subject says most of it: I would like to propose that the Arm targets default to -fno-common for performance and code-size reasons. In addition, GCC has also flipped the switch recently, see [1], so we would get the same behaviour as GCC which would also be nice. I've learned that the switch for GCC was relatively straightforward (for users), and there is some GNU documentation with Q&As to help toolchain users to transition if they find problems. I have put a draft patch up for review here: https://reviews.llvm.org/D75056. This definitely needs some doc changes (and I need to look at the tests), but first I would like to see how we feel about flipping the switch: any ideas, comments, objections?
>

Is there any reason to limit this to just the ARM targets?

-Tom

> Cheers,
> Sjoerd.
>
>
> [1] The GCC commit:
>
> commit 6271dd984d7f920d4fb17ad37af6a1f8e6b796dc
> Author: Wilco Dijkstra <[hidden email]>
> Date:   Wed Nov 20 16:29:23 2019 +0000
>
>     PR85678: Change default to -fno-common
>     GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
>     C feature which is not conforming with the latest C standards.  On many targets
>     this means global variable accesses have a codesize and performance penalty.
>     This applies to C code only, C++ code is not affected by -fcommon.  It is about
>     time to change the default.
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [ARM][AArch64] Default to -fno-common

suyash singh via cfe-dev
IMO, it would be less confusing to just change the default to false across the board. 

If anything should still use -fcommon by default, I'd want that to be based on vendor/OS, not CPU.

On Mon, Feb 24, 2020 at 11:38 AM Sjoerd Meijer via cfe-dev <[hidden email]> wrote:
>  Is there any reason to limit this to just the ARM targets?

No good reason other than that I don't know how beneficial this is for other targets.
But I definitely don't mind giving other targets the same treatment though while I am at it.

Cheers,
Sjoerd.


From: Tom Stellard <[hidden email]>
Sent: 24 February 2020 16:31
To: Sjoerd Meijer <[hidden email]>; [hidden email] Developers <[hidden email]>
Subject: Re: [cfe-dev] [ARM][AArch64] Default to -fno-common
 
On 02/24/2020 08:28 AM, Sjoerd Meijer via cfe-dev wrote:
> Hello,
>
> Subject says most of it: I would like to propose that the Arm targets default to -fno-common for performance and code-size reasons. In addition, GCC has also flipped the switch recently, see [1], so we would get the same behaviour as GCC which would also be nice. I've learned that the switch for GCC was relatively straightforward (for users), and there is some GNU documentation with Q&As to help toolchain users to transition if they find problems. I have put a draft patch up for review here: https://reviews.llvm.org/D75056. This definitely needs some doc changes (and I need to look at the tests), but first I would like to see how we feel about flipping the switch: any ideas, comments, objections?
>

Is there any reason to limit this to just the ARM targets?

-Tom

> Cheers,
> Sjoerd.
>
>
> [1] The GCC commit:
>
> commit 6271dd984d7f920d4fb17ad37af6a1f8e6b796dc
> Author: Wilco Dijkstra <[hidden email]>
> Date:   Wed Nov 20 16:29:23 2019 +0000
>
>     PR85678: Change default to -fno-common
>     GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
>     C feature which is not conforming with the latest C standards.  On many targets
>     this means global variable accesses have a codesize and performance penalty.
>     This applies to C code only, C++ code is not affected by -fcommon.  It is about
>     time to change the default.
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [ARM][AArch64] Default to -fno-common

suyash singh via cfe-dev
Thanks for your comments and suggestions. I think we can continue the discussion in D75056, and I will prepare a revision that enables this for all targets. 

Cheers,
Sjoerd.

From: James Y Knight <[hidden email]>
Sent: 24 February 2020 21:49
To: Sjoerd Meijer <[hidden email]>
Cc: Tom Stellard <[hidden email]>; [hidden email] Developers <[hidden email]>
Subject: Re: [cfe-dev] [ARM][AArch64] Default to -fno-common
 
IMO, it would be less confusing to just change the default to false across the board. 

If anything should still use -fcommon by default, I'd want that to be based on vendor/OS, not CPU.

On Mon, Feb 24, 2020 at 11:38 AM Sjoerd Meijer via cfe-dev <[hidden email]> wrote:
>  Is there any reason to limit this to just the ARM targets?

No good reason other than that I don't know how beneficial this is for other targets.
But I definitely don't mind giving other targets the same treatment though while I am at it.

Cheers,
Sjoerd.


From: Tom Stellard <[hidden email]>
Sent: 24 February 2020 16:31
To: Sjoerd Meijer <[hidden email]>; [hidden email] Developers <[hidden email]>
Subject: Re: [cfe-dev] [ARM][AArch64] Default to -fno-common
 
On 02/24/2020 08:28 AM, Sjoerd Meijer via cfe-dev wrote:
> Hello,
>
> Subject says most of it: I would like to propose that the Arm targets default to -fno-common for performance and code-size reasons. In addition, GCC has also flipped the switch recently, see [1], so we would get the same behaviour as GCC which would also be nice. I've learned that the switch for GCC was relatively straightforward (for users), and there is some GNU documentation with Q&As to help toolchain users to transition if they find problems. I have put a draft patch up for review here: https://reviews.llvm.org/D75056. This definitely needs some doc changes (and I need to look at the tests), but first I would like to see how we feel about flipping the switch: any ideas, comments, objections?
>

Is there any reason to limit this to just the ARM targets?

-Tom

> Cheers,
> Sjoerd.
>
>
> [1] The GCC commit:
>
> commit 6271dd984d7f920d4fb17ad37af6a1f8e6b796dc
> Author: Wilco Dijkstra <[hidden email]>
> Date:   Wed Nov 20 16:29:23 2019 +0000
>
>     PR85678: Change default to -fno-common
>     GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
>     C feature which is not conforming with the latest C standards.  On many targets
>     this means global variable accesses have a codesize and performance penalty.
>     This applies to C code only, C++ code is not affected by -fcommon.  It is about
>     time to change the default.
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [ARM][AArch64] Default to -fno-common

suyash singh via cfe-dev
I am in favor of this change (-fno-common is the sane default; GCC HEAD (future 11) will default to -fno-common),
and left some comments on https://reviews.llvm.org/D75056

But I have questions about the two claims.

For performance benefits:
   Is it because

   1. we missed some optimization for CommonLinkage
   2. CommonLinkage is considered interposable.

For code-size reasons:
   I think it may actually be a very minor regression. Why does a common
   symbol cost more bytes?

On 2020-02-25, Sjoerd Meijer via cfe-dev wrote:

>Thanks for your comments and suggestions. I think we can continue the discussion in D75056, and I will prepare a revision that enables this for all targets.
>
>Cheers,
>Sjoerd.
>________________________________
>From: James Y Knight <[hidden email]>
>Sent: 24 February 2020 21:49
>To: Sjoerd Meijer <[hidden email]>
>Cc: Tom Stellard <[hidden email]>; [hidden email] Developers <[hidden email]>
>Subject: Re: [cfe-dev] [ARM][AArch64] Default to -fno-common
>
>IMO, it would be less confusing to just change the default to false across the board.
>
>If anything should still use -fcommon by default, I'd want that to be based on vendor/OS, not CPU.
>
>On Mon, Feb 24, 2020 at 11:38 AM Sjoerd Meijer via cfe-dev <[hidden email]<mailto:[hidden email]>> wrote:
>>  Is there any reason to limit this to just the ARM targets?
>
>No good reason other than that I don't know how beneficial this is for other targets.
>But I definitely don't mind giving other targets the same treatment though while I am at it.
>
>Cheers,
>Sjoerd.
>
>________________________________
>From: Tom Stellard <[hidden email]<mailto:[hidden email]>>
>Sent: 24 February 2020 16:31
>To: Sjoerd Meijer <[hidden email]<mailto:[hidden email]>>; [hidden email]<mailto:[hidden email]> Developers <[hidden email]<mailto:[hidden email]>>
>Subject: Re: [cfe-dev] [ARM][AArch64] Default to -fno-common
>
>On 02/24/2020 08:28 AM, Sjoerd Meijer via cfe-dev wrote:
>> Hello,
>>
>> Subject says most of it: I would like to propose that the Arm targets default to -fno-common for performance and code-size reasons. In addition, GCC has also flipped the switch recently, see [1], so we would get the same behaviour as GCC which would also be nice. I've learned that the switch for GCC was relatively straightforward (for users), and there is some GNU documentation with Q&As to help toolchain users to transition if they find problems. I have put a draft patch up for review here: https://reviews.llvm.org/D75056. This definitely needs some doc changes (and I need to look at the tests), but first I would like to see how we feel about flipping the switch: any ideas, comments, objections?
>>
>
>Is there any reason to limit this to just the ARM targets?
>
>-Tom
>
>> Cheers,
>> Sjoerd.
>>
>>
>> [1] The GCC commit:
>>
>> commit 6271dd984d7f920d4fb17ad37af6a1f8e6b796dc
>> Author: Wilco Dijkstra <[hidden email]<mailto:[hidden email]>>
>> Date:   Wed Nov 20 16:29:23 2019 +0000
>>
>>     PR85678: Change default to -fno-common
>>     GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
>>     C feature which is not conforming with the latest C standards.  On many targets
>>     this means global variable accesses have a codesize and performance penalty.
>>     This applies to C code only, C++ code is not affected by -fcommon.  It is about
>>     time to change the default.
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [ARM][AArch64] Default to -fno-common

suyash singh via cfe-dev
Thanks for the comments, will address them tomorrow.

I thought potential performance benefits mainly comes from data not being placed in a common section, which is placed in a copy-on-write memory page.
And saying this from memory, I believe potential code-size savings comes from enabling the linker to do a better job in eliminating unused function sections, but I'd need to double check this tomorrow.

But perhaps these arguments are actually less relevant than it being a more sane and language conforming default (and it affects only C and not C++).

And because GCC has already flipped the switch with GCC10, (some) porting has been done, and some porting guides exists, for example:
and





From: cfe-dev <[hidden email]> on behalf of Fangrui Song via cfe-dev <[hidden email]>
Sent: 25 February 2020 17:47
To: cfe-dev <[hidden email]>
Subject: Re: [cfe-dev] [ARM][AArch64] Default to -fno-common
 
I am in favor of this change (-fno-common is the sane default; GCC HEAD (future 11) will default to -fno-common),
and left some comments on https://reviews.llvm.org/D75056

But I have questions about the two claims.

For performance benefits:
   Is it because

   1. we missed some optimization for CommonLinkage
   2. CommonLinkage is considered interposable.

For code-size reasons:
   I think it may actually be a very minor regression. Why does a common
   symbol cost more bytes?

On 2020-02-25, Sjoerd Meijer via cfe-dev wrote:
>Thanks for your comments and suggestions. I think we can continue the discussion in D75056, and I will prepare a revision that enables this for all targets.
>
>Cheers,
>Sjoerd.
>________________________________
>From: James Y Knight <[hidden email]>
>Sent: 24 February 2020 21:49
>To: Sjoerd Meijer <[hidden email]>
>Cc: Tom Stellard <[hidden email]>; [hidden email] Developers <[hidden email]>
>Subject: Re: [cfe-dev] [ARM][AArch64] Default to -fno-common
>
>IMO, it would be less confusing to just change the default to false across the board.
>
>If anything should still use -fcommon by default, I'd want that to be based on vendor/OS, not CPU.
>
>On Mon, Feb 24, 2020 at 11:38 AM Sjoerd Meijer via cfe-dev <[hidden email]<mailto:[hidden email]>> wrote:
>>  Is there any reason to limit this to just the ARM targets?
>
>No good reason other than that I don't know how beneficial this is for other targets.
>But I definitely don't mind giving other targets the same treatment though while I am at it.
>
>Cheers,
>Sjoerd.
>
>________________________________
>From: Tom Stellard <[hidden email]<mailto:[hidden email]>>
>Sent: 24 February 2020 16:31
>To: Sjoerd Meijer <[hidden email]<mailto:[hidden email]>>; [hidden email]<mailto:[hidden email]> Developers <[hidden email]<mailto:[hidden email]>>
>Subject: Re: [cfe-dev] [ARM][AArch64] Default to -fno-common
>
>On 02/24/2020 08:28 AM, Sjoerd Meijer via cfe-dev wrote:
>> Hello,
>>
>> Subject says most of it: I would like to propose that the Arm targets default to -fno-common for performance and code-size reasons. In addition, GCC has also flipped the switch recently, see [1], so we would get the same behaviour as GCC which would also be nice. I've learned that the switch for GCC was relatively straightforward (for users), and there is some GNU documentation with Q&As to help toolchain users to transition if they find problems. I have put a draft patch up for review here: https://reviews.llvm.org/D75056. This definitely needs some doc changes (and I need to look at the tests), but first I would like to see how we feel about flipping the switch: any ideas, comments, objections?
>>
>
>Is there any reason to limit this to just the ARM targets?
>
>-Tom
>
>> Cheers,
>> Sjoerd.
>>
>>
>> [1] The GCC commit:
>>
>> commit 6271dd984d7f920d4fb17ad37af6a1f8e6b796dc
>> Author: Wilco Dijkstra <[hidden email]<mailto:[hidden email]>>
>> Date:   Wed Nov 20 16:29:23 2019 +0000
>>
>>     PR85678: Change default to -fno-common
>>     GCC currently defaults to -fcommon.  As discussed in the PR, this is an ancient
>>     C feature which is not conforming with the latest C standards.  On many targets
>>     this means global variable accesses have a codesize and performance penalty.
>>     This applies to C code only, C++ code is not affected by -fcommon.  It is about
>>     time to change the default.
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [ARM][AArch64] Default to -fno-common

suyash singh via cfe-dev
In reply to this post by suyash singh via cfe-dev
From my experience, the most important optimization that doesn't work on "common" definitions is GlobalMerge.

-Eli

> -----Original Message-----
> From: cfe-dev <[hidden email]> On Behalf Of Fangrui Song via
> cfe-dev
> Sent: Tuesday, February 25, 2020 9:48 AM
> To: cfe-dev <[hidden email]>
> Subject: [EXT] Re: [cfe-dev] [ARM][AArch64] Default to -fno-common
>
> I am in favor of this change (-fno-common is the sane default; GCC HEAD (future
> 11) will default to -fno-common),
> and left some comments on https://reviews.llvm.org/D75056
>
> But I have questions about the two claims.
>
> For performance benefits:
>    Is it because
>
>    1. we missed some optimization for CommonLinkage
>    2. CommonLinkage is considered interposable.
>
> For code-size reasons:
>    I think it may actually be a very minor regression. Why does a common
>    symbol cost more bytes?
>
> On 2020-02-25, Sjoerd Meijer via cfe-dev wrote:
> >Thanks for your comments and suggestions. I think we can continue the
> discussion in D75056, and I will prepare a revision that enables this for all
> targets.
> >
> >Cheers,
> >Sjoerd.
> >________________________________
> >From: James Y Knight <[hidden email]>
> >Sent: 24 February 2020 21:49
> >To: Sjoerd Meijer <[hidden email]>
> >Cc: Tom Stellard <[hidden email]>; [hidden email] Developers
> <[hidden email]>
> >Subject: Re: [cfe-dev] [ARM][AArch64] Default to -fno-common
> >
> >IMO, it would be less confusing to just change the default to false across the
> board.
> >
> >If anything should still use -fcommon by default, I'd want that to be based on
> vendor/OS, not CPU.
> >
> >On Mon, Feb 24, 2020 at 11:38 AM Sjoerd Meijer via cfe-dev <cfe-
> [hidden email]<mailto:[hidden email]>> wrote:
> >>  Is there any reason to limit this to just the ARM targets?
> >
> >No good reason other than that I don't know how beneficial this is for other
> targets.
> >But I definitely don't mind giving other targets the same treatment though
> while I am at it.
> >
> >Cheers,
> >Sjoerd.
> >
> >________________________________
> >From: Tom Stellard <[hidden email]<mailto:[hidden email]>>
> >Sent: 24 February 2020 16:31
> >To: Sjoerd Meijer
> <[hidden email]<mailto:[hidden email]>>; cfe-
> [hidden email]<mailto:[hidden email]> Developers <cfe-
> [hidden email]<mailto:[hidden email]>>
> >Subject: Re: [cfe-dev] [ARM][AArch64] Default to -fno-common
> >
> >On 02/24/2020 08:28 AM, Sjoerd Meijer via cfe-dev wrote:
> >> Hello,
> >>
> >> Subject says most of it: I would like to propose that the Arm targets default
> to -fno-common for performance and code-size reasons. In addition, GCC has
> also flipped the switch recently, see [1], so we would get the same behaviour as
> GCC which would also be nice. I've learned that the switch for GCC was
> relatively straightforward (for users), and there is some GNU documentation
> with Q&As to help toolchain users to transition if they find problems. I have put
> a draft patch up for review here: https://reviews.llvm.org/D75056. This
> definitely needs some doc changes (and I need to look at the tests), but first I
> would like to see how we feel about flipping the switch: any ideas, comments,
> objections?
> >>
> >
> >Is there any reason to limit this to just the ARM targets?
> >
> >-Tom
> >
> >> Cheers,
> >> Sjoerd.
> >>
> >>
> >> [1] The GCC commit:
> >>
> >> commit 6271dd984d7f920d4fb17ad37af6a1f8e6b796dc
> >> Author: Wilco Dijkstra <[hidden email]<mailto:[hidden email]>>
> >> Date:   Wed Nov 20 16:29:23 2019 +0000
> >>
> >>     PR85678: Change default to -fno-common
> >>     GCC currently defaults to -fcommon.  As discussed in the PR, this is an
> ancient
> >>     C feature which is not conforming with the latest C standards.  On many
> targets
> >>     this means global variable accesses have a codesize and performance
> penalty.
> >>     This applies to C code only, C++ code is not affected by -fcommon.  It is
> about
> >>     time to change the default.
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev