[0/5] remove usage of register keyword in C
mbox series

Message ID 20180731163059.27085-1-stephen@networkplumber.org
Headers show
Series
  • remove usage of register keyword in C
Related show

Message

Stephen Hemminger July 31, 2018, 4:30 p.m. UTC
Declaring variables as register in C is a leftover from an earlier
era (like cassette tape decks in cars).

Stephen Hemminger (5):
  qat: remove redundant C register keyword
  qede: remove register from declaraitons
  ark: remove register keyword
  mlx5: no need for register keyword
  mlx4: remove redunant register keyword

 drivers/common/qat/qat_qp.c     | 10 +++++-----
 drivers/crypto/qat/qat_sym.c    |  2 +-
 drivers/net/ark/ark_ethdev_rx.c |  4 ++--
 drivers/net/mlx4/mlx4_mr.c      |  2 +-
 drivers/net/mlx5/mlx5_mr.c      |  2 +-
 drivers/net/qede/qede_rxtx.c    |  8 ++++----
 6 files changed, 14 insertions(+), 14 deletions(-)

Comments

Adrien Mazarguil July 31, 2018, 4:48 p.m. UTC | #1
On Tue, Jul 31, 2018 at 09:30:54AM -0700, Stephen Hemminger wrote:
> Declaring variables as register in C is a leftover from an earlier
> era (like cassette tape decks in cars).

I don't agree here. It's a hint for compilers and developers that the
address of such variables won't be needed (and cannot be taken) to enable
whatever optimizations are possible knowing this.

Somewhat like inline functions, it's not a forced optimization, just a
useful hint that shouldn't hurt if used wisely.

Besides, cassette decks are not dead yet :)
Stephen Hemminger July 31, 2018, 6:07 p.m. UTC | #2
On Tue, 31 Jul 2018 18:48:40 +0200
Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:

> On Tue, Jul 31, 2018 at 09:30:54AM -0700, Stephen Hemminger wrote:
> > Declaring variables as register in C is a leftover from an earlier
> > era (like cassette tape decks in cars).  
> 
> I don't agree here. It's a hint for compilers and developers that the
> address of such variables won't be needed (and cannot be taken) to enable
> whatever optimizations are possible knowing this.
> 
> Somewhat like inline functions, it's not a forced optimization, just a
> useful hint that shouldn't hurt if used wisely.
> 
> Besides, cassette decks are not dead yet :)

If you look at the code, that is not how register is being used (ie. don't take
address of this). It seems like an attempt at optimization.
Matan Azrad Aug. 1, 2018, 10:18 a.m. UTC | #3
Hi Stephen

Can you elaborate more?
Can you add references?


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Tuesday, July 31, 2018 7:31 PM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Subject: [dpdk-dev] [PATCH 0/5] remove usage of register keyword in C
> 
> Declaring variables as register in C is a leftover from an earlier era (like
> cassette tape decks in cars).
> 
> Stephen Hemminger (5):
>   qat: remove redundant C register keyword
>   qede: remove register from declaraitons
>   ark: remove register keyword
>   mlx5: no need for register keyword
>   mlx4: remove redunant register keyword
> 
>  drivers/common/qat/qat_qp.c     | 10 +++++-----
>  drivers/crypto/qat/qat_sym.c    |  2 +-
>  drivers/net/ark/ark_ethdev_rx.c |  4 ++--
>  drivers/net/mlx4/mlx4_mr.c      |  2 +-
>  drivers/net/mlx5/mlx5_mr.c      |  2 +-
>  drivers/net/qede/qede_rxtx.c    |  8 ++++----
>  6 files changed, 14 insertions(+), 14 deletions(-)
> 
> --
> 2.18.0
Yongseok Koh Aug. 1, 2018, 6:03 p.m. UTC | #4
> On Jul 31, 2018, at 11:07 AM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> On Tue, 31 Jul 2018 18:48:40 +0200
> Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> 
>> On Tue, Jul 31, 2018 at 09:30:54AM -0700, Stephen Hemminger wrote:
>>> Declaring variables as register in C is a leftover from an earlier
>>> era (like cassette tape decks in cars).  
>> 
>> I don't agree here. It's a hint for compilers and developers that the
>> address of such variables won't be needed (and cannot be taken) to enable
>> whatever optimizations are possible knowing this.
>> 
>> Somewhat like inline functions, it's not a forced optimization, just a
>> useful hint that shouldn't hurt if used wisely.
>> 
>> Besides, cassette decks are not dead yet :)
> 
> If you look at the code, that is not how register is being used (ie. don't take
> address of this). It seems like an attempt at optimization.

I know compilers are smart enough and the occurrences in mlx4/5 were made from
my old fashioned habit. But, I don't see any urgency to push this patch in RC
stage even though I'm 99% sure that it is harmless. And in general I don't even
understand why we can't live with that if it isn't harmful (or a violation) but
informative. I mean no badness but at least one goodness :-)

Thanks,
Yongseok
Stephen Hemminger Aug. 1, 2018, 9:03 p.m. UTC | #5
On Wed, 1 Aug 2018 18:03:04 +0000
Yongseok Koh <yskoh@mellanox.com> wrote:

> > On Jul 31, 2018, at 11:07 AM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> > 
> > On Tue, 31 Jul 2018 18:48:40 +0200
> > Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> >   
> >> On Tue, Jul 31, 2018 at 09:30:54AM -0700, Stephen Hemminger wrote:  
> >>> Declaring variables as register in C is a leftover from an earlier
> >>> era (like cassette tape decks in cars).    
> >> 
> >> I don't agree here. It's a hint for compilers and developers that the
> >> address of such variables won't be needed (and cannot be taken) to enable
> >> whatever optimizations are possible knowing this.
> >> 
> >> Somewhat like inline functions, it's not a forced optimization, just a
> >> useful hint that shouldn't hurt if used wisely.
> >> 
> >> Besides, cassette decks are not dead yet :)  
> > 
> > If you look at the code, that is not how register is being used (ie. don't take
> > address of this). It seems like an attempt at optimization.  
> 
> I know compilers are smart enough and the occurrences in mlx4/5 were made from
> my old fashioned habit. But, I don't see any urgency to push this patch in RC
> stage even though I'm 99% sure that it is harmless. And in general I don't even
> understand why we can't live with that if it isn't harmful (or a violation) but
> informative. I mean no badness but at least one goodness :-)
> 
> Thanks,
> Yongseok
> 

Sure, this is intended for next release not rc stage.
Just trying to clean up code base where I see it.
Ferruh Yigit Aug. 23, 2018, 1:07 p.m. UTC | #6
On 8/1/2018 10:03 PM, Stephen Hemminger wrote:
> On Wed, 1 Aug 2018 18:03:04 +0000
> Yongseok Koh <yskoh@mellanox.com> wrote:
> 
>>> On Jul 31, 2018, at 11:07 AM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>>>
>>> On Tue, 31 Jul 2018 18:48:40 +0200
>>> Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
>>>   
>>>> On Tue, Jul 31, 2018 at 09:30:54AM -0700, Stephen Hemminger wrote:  
>>>>> Declaring variables as register in C is a leftover from an earlier
>>>>> era (like cassette tape decks in cars).    
>>>>
>>>> I don't agree here. It's a hint for compilers and developers that the
>>>> address of such variables won't be needed (and cannot be taken) to enable
>>>> whatever optimizations are possible knowing this.
>>>>
>>>> Somewhat like inline functions, it's not a forced optimization, just a
>>>> useful hint that shouldn't hurt if used wisely.
>>>>
>>>> Besides, cassette decks are not dead yet :)  
>>>
>>> If you look at the code, that is not how register is being used (ie. don't take
>>> address of this). It seems like an attempt at optimization.  
>>
>> I know compilers are smart enough and the occurrences in mlx4/5 were made from
>> my old fashioned habit. But, I don't see any urgency to push this patch in RC
>> stage even though I'm 99% sure that it is harmless. And in general I don't even
>> understand why we can't live with that if it isn't harmful (or a violation) but
>> informative. I mean no badness but at least one goodness :-)
>>
>> Thanks,
>> Yongseok
>>
> 
> Sure, this is intended for next release not rc stage.
> Just trying to clean up code base where I see it.

I agree with Yongseok, at worst they show the intention of the developer, I
don't see motivation to remove them unless they are doing something wrong, which
seems not the reason of this patch.

And although I found some information that says "register" ignored completely
for gcc, I can see it differs when optimization disabled.
I am not saying practically it differs, since we enable optimization expect from
debugging, most probably there is no practical difference between having the
keyword or not, but what I am trying to say is it not completely ignored either.
Ferruh Yigit Oct. 9, 2018, 9:19 a.m. UTC | #7
On 8/23/2018 2:07 PM, Ferruh Yigit wrote:
> On 8/1/2018 10:03 PM, Stephen Hemminger wrote:
>> On Wed, 1 Aug 2018 18:03:04 +0000
>> Yongseok Koh <yskoh@mellanox.com> wrote:
>>
>>>> On Jul 31, 2018, at 11:07 AM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>>>>
>>>> On Tue, 31 Jul 2018 18:48:40 +0200
>>>> Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
>>>>   
>>>>> On Tue, Jul 31, 2018 at 09:30:54AM -0700, Stephen Hemminger wrote:  
>>>>>> Declaring variables as register in C is a leftover from an earlier
>>>>>> era (like cassette tape decks in cars).    
>>>>>
>>>>> I don't agree here. It's a hint for compilers and developers that the
>>>>> address of such variables won't be needed (and cannot be taken) to enable
>>>>> whatever optimizations are possible knowing this.
>>>>>
>>>>> Somewhat like inline functions, it's not a forced optimization, just a
>>>>> useful hint that shouldn't hurt if used wisely.
>>>>>
>>>>> Besides, cassette decks are not dead yet :)  
>>>>
>>>> If you look at the code, that is not how register is being used (ie. don't take
>>>> address of this). It seems like an attempt at optimization.  
>>>
>>> I know compilers are smart enough and the occurrences in mlx4/5 were made from
>>> my old fashioned habit. But, I don't see any urgency to push this patch in RC
>>> stage even though I'm 99% sure that it is harmless. And in general I don't even
>>> understand why we can't live with that if it isn't harmful (or a violation) but
>>> informative. I mean no badness but at least one goodness :-)
>>>
>>> Thanks,
>>> Yongseok
>>>
>>
>> Sure, this is intended for next release not rc stage.
>> Just trying to clean up code base where I see it.
> 
> I agree with Yongseok, at worst they show the intention of the developer, I
> don't see motivation to remove them unless they are doing something wrong, which
> seems not the reason of this patch.
> 
> And although I found some information that says "register" ignored completely
> for gcc, I can see it differs when optimization disabled.
> I am not saying practically it differs, since we enable optimization expect from
> debugging, most probably there is no practical difference between having the
> keyword or not, but what I am trying to say is it not completely ignored either.

I am for marking this set as rejected, any objection?
Tom Barbette Jan. 31, 2019, 8:02 a.m. UTC | #8
Hi all,

Has there been any change regarding this? I'm still at DPDK 18.11. Maybe 
automatically add -Wno-register when C++17 is enabled? Or have a some 
register macro which gets undefined if C++17 is enabled?

The "warning: ISO C++1z does not allow ‘register’ storage class 
specifier" is annoying. And vim always goes to some DPDK header when 
":make" fails because of the warning...

Thanks,
Tom

On 2018-10-09 11:19, Ferruh Yigit wrote:
> On 8/23/2018 2:07 PM, Ferruh Yigit wrote:
>> On 8/1/2018 10:03 PM, Stephen Hemminger wrote:
>>> On Wed, 1 Aug 2018 18:03:04 +0000
>>> Yongseok Koh <yskoh@mellanox.com> wrote:
>>>
>>>>> On Jul 31, 2018, at 11:07 AM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>>>>>
>>>>> On Tue, 31 Jul 2018 18:48:40 +0200
>>>>> Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
>>>>>    
>>>>>> On Tue, Jul 31, 2018 at 09:30:54AM -0700, Stephen Hemminger wrote:
>>>>>>> Declaring variables as register in C is a leftover from an earlier
>>>>>>> era (like cassette tape decks in cars).
>>>>>>
>>>>>> I don't agree here. It's a hint for compilers and developers that the
>>>>>> address of such variables won't be needed (and cannot be taken) to enable
>>>>>> whatever optimizations are possible knowing this.
>>>>>>
>>>>>> Somewhat like inline functions, it's not a forced optimization, just a
>>>>>> useful hint that shouldn't hurt if used wisely.
>>>>>>
>>>>>> Besides, cassette decks are not dead yet :)
>>>>>
>>>>> If you look at the code, that is not how register is being used (ie. don't take
>>>>> address of this). It seems like an attempt at optimization.
>>>>
>>>> I know compilers are smart enough and the occurrences in mlx4/5 were made from
>>>> my old fashioned habit. But, I don't see any urgency to push this patch in RC
>>>> stage even though I'm 99% sure that it is harmless. And in general I don't even
>>>> understand why we can't live with that if it isn't harmful (or a violation) but
>>>> informative. I mean no badness but at least one goodness :-)
>>>>
>>>> Thanks,
>>>> Yongseok
>>>>
>>>
>>> Sure, this is intended for next release not rc stage.
>>> Just trying to clean up code base where I see it.
>>
>> I agree with Yongseok, at worst they show the intention of the developer, I
>> don't see motivation to remove them unless they are doing something wrong, which
>> seems not the reason of this patch.
>>
>> And although I found some information that says "register" ignored completely
>> for gcc, I can see it differs when optimization disabled.
>> I am not saying practically it differs, since we enable optimization expect from
>> debugging, most probably there is no practical difference between having the
>> keyword or not, but what I am trying to say is it not completely ignored either.
> 
> I am for marking this set as rejected, any objection?
>
Bruce Richardson Jan. 31, 2019, 9:11 a.m. UTC | #9
On Thu, Jan 31, 2019 at 09:02:36AM +0100, Tom Barbette wrote:
> Hi all,
> 
> Has there been any change regarding this? I'm still at DPDK 18.11. Maybe
> automatically add -Wno-register when C++17 is enabled? Or have a some
> register macro which gets undefined if C++17 is enabled?
> 
> The "warning: ISO C++1z does not allow ‘register’ storage class specifier"
> is annoying. And vim always goes to some DPDK header when ":make" fails
> because of the warning...
> 
> Thanks,
> Tom
> 
What header is that? From what I see the patchset only makes changes to .c
files rather than any .h files, so not sure it would help in your case.

For what it's worth on the general discussion, I'm in favour of applying
this patchset. I view marking variables as "register" as completely
unncessary. If someone can demonstrate a place where it actually makes a
difference, then we can keep that use of the keywork, otherwise I think the
code is as well off without it.

/Bruce
Tom Barbette Jan. 31, 2019, 9:39 a.m. UTC | #10
On 2019-01-31 10:11, Bruce Richardson wrote:
> What header is that? From what I see the patchset only makes changes to .c
> files rather than any .h files, so not sure it would help in your case.

Yes you're right. There are other occurrences of register indeed.

I got the warning from rte_common.h:308 : rte_combine64ms1b(register 
uint64_t v) but it may not be the only one?

Tom
Wiles, Keith Jan. 31, 2019, 5:34 p.m. UTC | #11
I agree using register for todays compilers is unnecessary and can actually be wrong in some cases. The compilers can pick the correct registers better then we can normally and restricting the compiler makes no sense. 

Sent from my iPhone

> On Jan 31, 2019, at 3:11 AM, Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
>> On Thu, Jan 31, 2019 at 09:02:36AM +0100, Tom Barbette wrote:
>> Hi all,
>> 
>> Has there been any change regarding this? I'm still at DPDK 18.11. Maybe
>> automatically add -Wno-register when C++17 is enabled? Or have a some
>> register macro which gets undefined if C++17 is enabled?
>> 
>> The "warning: ISO C++1z does not allow ‘register’ storage class specifier"
>> is annoying. And vim always goes to some DPDK header when ":make" fails
>> because of the warning...
>> 
>> Thanks,
>> Tom
>> 
> What header is that? From what I see the patchset only makes changes to .c
> files rather than any .h files, so not sure it would help in your case.
> 
> For what it's worth on the general discussion, I'm in favour of applying
> this patchset. I view marking variables as "register" as completely
> unncessary. If someone can demonstrate a place where it actually makes a
> difference, then we can keep that use of the keywork, otherwise I think the
> code is as well off without it.
> 
> /Bruce