[PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller

[PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

Don't populate the array ext_div on the stack but instead it
static const. Makes the object code smaller by 118 bytes:

Before:
   text    data    bss     dec    hex filename
  39449   17500    128   57077   def5 ./drivers/gpu/drm/bridge/tc358767.o

After:
   text    data    bss     dec    hex filename
  39235   17596    128   56959   de7f ./drivers/gpu/drm/bridge/tc358767.o

(gcc version 10.3.0)

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 23a6f90b694b..599c23759400 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -468,7 +468,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
 	int div, best_div = 1;
 	int mul, best_mul = 1;
 	int delta, best_delta;
-	int ext_div[] = {1, 2, 3, 5, 7};
+	static const int ext_div[] = {1, 2, 3, 5, 7};
 	int best_pixelclock = 0;
 	int vco_hi = 0;
 	u32 pxl_pllparam;
-- 
2.32.0

Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller

[Joe Perches]

On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Don't populate the array ext_div on the stack but instead it
> static const. Makes the object code smaller by 118 bytes:
> 
> Before:
>    text    data    bss     dec    hex filename
>   39449   17500    128   57077   def5 ./drivers/gpu/drm/bridge/tc358767.o
> 
> After:
>    text    data    bss     dec    hex filename
>   39235   17596    128   56959   de7f ./drivers/gpu/drm/bridge/tc358767.o

Why is text smaller and data larger with this change?

> 
> (gcc version 10.3.0)
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 23a6f90b694b..599c23759400 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -468,7 +468,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
>  	int div, best_div = 1;
>  	int mul, best_mul = 1;
>  	int delta, best_delta;
> -	int ext_div[] = {1, 2, 3, 5, 7};
> +	static const int ext_div[] = {1, 2, 3, 5, 7};
>  	int best_pixelclock = 0;
>  	int vco_hi = 0;
>  	u32 pxl_pllparam;

Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller

[Joe Perches]

On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Don't populate the array ext_div on the stack but instead it
> static const. Makes the object code smaller by 118 bytes:
> 
> Before:
>    text    data    bss     dec    hex filename
>   39449   17500    128   57077   def5 ./drivers/gpu/drm/bridge/tc358767.o
> 
> After:
>    text    data    bss     dec    hex filename
>   39235   17596    128   56959   de7f ./drivers/gpu/drm/bridge/tc358767.o

Why is text smaller and data larger with this change?

> 
> (gcc version 10.3.0)
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 23a6f90b694b..599c23759400 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -468,7 +468,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
>  	int div, best_div = 1;
>  	int mul, best_mul = 1;
>  	int delta, best_delta;
> -	int ext_div[] = {1, 2, 3, 5, 7};
> +	static const int ext_div[] = {1, 2, 3, 5, 7};
>  	int best_pixelclock = 0;
>  	int vco_hi = 0;
>  	u32 pxl_pllparam;

Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller

[Colin Ian King]

On 19/08/2021 14:51, Joe Perches wrote:
> On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Don't populate the array ext_div on the stack but instead it
>> static const. Makes the object code smaller by 118 bytes:
>>
>> Before:
>>    text    data    bss     dec    hex filename
>>   39449   17500    128   57077   def5 ./drivers/gpu/drm/bridge/tc358767.o
>>
>> After:
>>    text    data    bss     dec    hex filename
>>   39235   17596    128   56959   de7f ./drivers/gpu/drm/bridge/tc358767.o
> 
> Why is text smaller and data larger with this change?

There are less instructions being used with the change since it's not
shoving the array data onto the stack at run time. Instead the array is
being stored in the data section and there is less object code required
to access the data.

Colin

> 
>>
>> (gcc version 10.3.0)
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/gpu/drm/bridge/tc358767.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index 23a6f90b694b..599c23759400 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -468,7 +468,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
>>  	int div, best_div = 1;
>>  	int mul, best_mul = 1;
>>  	int delta, best_delta;
>> -	int ext_div[] = {1, 2, 3, 5, 7};
>> +	static const int ext_div[] = {1, 2, 3, 5, 7};
>>  	int best_pixelclock = 0;
>>  	int vco_hi = 0;
>>  	u32 pxl_pllparam;
> 
> 

Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller

[Colin Ian King]

On 19/08/2021 14:51, Joe Perches wrote:
> On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Don't populate the array ext_div on the stack but instead it
>> static const. Makes the object code smaller by 118 bytes:
>>
>> Before:
>>    text    data    bss     dec    hex filename
>>   39449   17500    128   57077   def5 ./drivers/gpu/drm/bridge/tc358767.o
>>
>> After:
>>    text    data    bss     dec    hex filename
>>   39235   17596    128   56959   de7f ./drivers/gpu/drm/bridge/tc358767.o
> 
> Why is text smaller and data larger with this change?

There are less instructions being used with the change since it's not
shoving the array data onto the stack at run time. Instead the array is
being stored in the data section and there is less object code required
to access the data.

Colin

> 
>>
>> (gcc version 10.3.0)
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/gpu/drm/bridge/tc358767.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index 23a6f90b694b..599c23759400 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -468,7 +468,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
>>  	int div, best_div = 1;
>>  	int mul, best_mul = 1;
>>  	int delta, best_delta;
>> -	int ext_div[] = {1, 2, 3, 5, 7};
>> +	static const int ext_div[] = {1, 2, 3, 5, 7};
>>  	int best_pixelclock = 0;
>>  	int vco_hi = 0;
>>  	u32 pxl_pllparam;
> 
> 

Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller

[Joe Perches]

On Thu, 2021-08-19 at 14:54 +0100, Colin Ian King wrote:
> On 19/08/2021 14:51, Joe Perches wrote:
> > On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote:
> > > From: Colin Ian King <colin.king@canonical.com>
> > > 
> > > Don't populate the array ext_div on the stack but instead it
> > > static const. Makes the object code smaller by 118 bytes:
> > > 
> > > Before:
> > >    text    data    bss     dec    hex filename
> > >   39449   17500    128   57077   def5 ./drivers/gpu/drm/bridge/tc358767.o
> > > 
> > > After:
> > >    text    data    bss     dec    hex filename
> > >   39235   17596    128   56959   de7f ./drivers/gpu/drm/bridge/tc358767.o
> > 
> > Why is text smaller and data larger with this change?
> 
> There are less instructions being used with the change since it's not
> shoving the array data onto the stack at run time. Instead the array is
> being stored in the data section and there is less object code required
> to access the data.

Ah.  It's really because it's not a minimal compilation ala defconfig.

I think you should really stop making these size comparisons with
.config uses that are not based on a defconfig as a whole lot of other
things are going on.

Please notice that the object sizes are significantly smaller below:

So with an x86-64 defconfig and this compilation unit enabled with
CONFIG_OF enabled and CONFIG_DRM_TOSHIBA_TC358767=y, with gcc 10.3
and this change the object size actually increases a bit.

$ size drivers/gpu/drm/bridge/tc358767.o*
   text	   data	    bss	    dec	    hex	filename
  13554	    268	      1	  13823	   35ff	drivers/gpu/drm/bridge/tc358767.o.new
  13548	    268	      1	  13817	   35f9	drivers/gpu/drm/bridge/tc358767.o.old

objdump -h shows these differences:

.old:
  0 .text         00001e1f  0000000000000000  0000000000000000  00000040  2**4
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
[...]
 14 .rodata       000005ae  0000000000000000  0000000000000000  000046e0  2**5
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA

.new:
  0 .text         00001e05  0000000000000000  0000000000000000  00000040  2**4
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
[...]
 11 .rodata       000005ce  0000000000000000  0000000000000000  00004600  2**5
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA

cheers, Joe

Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller

[Joe Perches]

On Thu, 2021-08-19 at 14:54 +0100, Colin Ian King wrote:
> On 19/08/2021 14:51, Joe Perches wrote:
> > On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote:
> > > From: Colin Ian King <colin.king@canonical.com>
> > > 
> > > Don't populate the array ext_div on the stack but instead it
> > > static const. Makes the object code smaller by 118 bytes:
> > > 
> > > Before:
> > >    text    data    bss     dec    hex filename
> > >   39449   17500    128   57077   def5 ./drivers/gpu/drm/bridge/tc358767.o
> > > 
> > > After:
> > >    text    data    bss     dec    hex filename
> > >   39235   17596    128   56959   de7f ./drivers/gpu/drm/bridge/tc358767.o
> > 
> > Why is text smaller and data larger with this change?
> 
> There are less instructions being used with the change since it's not
> shoving the array data onto the stack at run time. Instead the array is
> being stored in the data section and there is less object code required
> to access the data.

Ah.  It's really because it's not a minimal compilation ala defconfig.

I think you should really stop making these size comparisons with
.config uses that are not based on a defconfig as a whole lot of other
things are going on.

Please notice that the object sizes are significantly smaller below:

So with an x86-64 defconfig and this compilation unit enabled with
CONFIG_OF enabled and CONFIG_DRM_TOSHIBA_TC358767=y, with gcc 10.3
and this change the object size actually increases a bit.

$ size drivers/gpu/drm/bridge/tc358767.o*
   text	   data	    bss	    dec	    hex	filename
  13554	    268	      1	  13823	   35ff	drivers/gpu/drm/bridge/tc358767.o.new
  13548	    268	      1	  13817	   35f9	drivers/gpu/drm/bridge/tc358767.o.old

objdump -h shows these differences:

.old:
  0 .text         00001e1f  0000000000000000  0000000000000000  00000040  2**4
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
[...]
 14 .rodata       000005ae  0000000000000000  0000000000000000  000046e0  2**5
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA

.new:
  0 .text         00001e05  0000000000000000  0000000000000000  00000040  2**4
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
[...]
 11 .rodata       000005ce  0000000000000000  0000000000000000  00004600  2**5
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA

cheers, Joe

Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller

[Colin Ian King]

On 19/08/2021 15:40, Joe Perches wrote:
> On Thu, 2021-08-19 at 14:54 +0100, Colin Ian King wrote:
>> On 19/08/2021 14:51, Joe Perches wrote:
>>> On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote:
>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>
>>>> Don't populate the array ext_div on the stack but instead it
>>>> static const. Makes the object code smaller by 118 bytes:
>>>>
>>>> Before:
>>>>    text    data    bss     dec    hex filename
>>>>   39449   17500    128   57077   def5 ./drivers/gpu/drm/bridge/tc358767.o
>>>>
>>>> After:
>>>>    text    data    bss     dec    hex filename
>>>>   39235   17596    128   56959   de7f ./drivers/gpu/drm/bridge/tc358767.o
>>>
>>> Why is text smaller and data larger with this change?
>>
>> There are less instructions being used with the change since it's not
>> shoving the array data onto the stack at run time. Instead the array is
>> being stored in the data section and there is less object code required
>> to access the data.
> 
> Ah.  It's really because it's not a minimal compilation ala defconfig >
> I think you should really stop making these size comparisons with
> .config uses that are not based on a defconfig as a whole lot of other
> things are going on.

I'm using allmodconfig, which I believe is a legitimate configuration,
especially since distros so build kernels with lots of modules.
I'll double check on this though in case I've made a mistake.

> 
> Please notice that the object sizes are significantly smaller below:
> 
> So with an x86-64 defconfig and this compilation unit enabled with
> CONFIG_OF enabled and CONFIG_DRM_TOSHIBA_TC358767=y, with gcc 10.3
> and this change the object size actually increases a bit.
> 
> $ size drivers/gpu/drm/bridge/tc358767.o*
>    text	   data	    bss	    dec	    hex	filename
>   13554	    268	      1	  13823	   35ff	drivers/gpu/drm/bridge/tc358767.o.new
>   13548	    268	      1	  13817	   35f9	drivers/gpu/drm/bridge/tc358767.o.old>
> objdump -h shows these differences:
> 
> .old:
>   0 .text         00001e1f  0000000000000000  0000000000000000  00000040  2**4
>                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
> [...]
>  14 .rodata       000005ae  0000000000000000  0000000000000000  000046e0  2**5
>                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
> 
> .new:
>   0 .text         00001e05  0000000000000000  0000000000000000  00000040  2**4
>                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
> [...]
>  11 .rodata       000005ce  0000000000000000  0000000000000000  00004600  2**5
>                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA

ACK. Understood.  Even so, it still makes sense for these kind of
janitorial changes as it makes sense to constify arrays when they are
read-only and making them static is sensible for const data.

> 
> cheers, Joe
> 

Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller

[Colin Ian King]

On 19/08/2021 15:40, Joe Perches wrote:
> On Thu, 2021-08-19 at 14:54 +0100, Colin Ian King wrote:
>> On 19/08/2021 14:51, Joe Perches wrote:
>>> On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote:
>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>
>>>> Don't populate the array ext_div on the stack but instead it
>>>> static const. Makes the object code smaller by 118 bytes:
>>>>
>>>> Before:
>>>>    text    data    bss     dec    hex filename
>>>>   39449   17500    128   57077   def5 ./drivers/gpu/drm/bridge/tc358767.o
>>>>
>>>> After:
>>>>    text    data    bss     dec    hex filename
>>>>   39235   17596    128   56959   de7f ./drivers/gpu/drm/bridge/tc358767.o
>>>
>>> Why is text smaller and data larger with this change?
>>
>> There are less instructions being used with the change since it's not
>> shoving the array data onto the stack at run time. Instead the array is
>> being stored in the data section and there is less object code required
>> to access the data.
> 
> Ah.  It's really because it's not a minimal compilation ala defconfig >
> I think you should really stop making these size comparisons with
> .config uses that are not based on a defconfig as a whole lot of other
> things are going on.

I'm using allmodconfig, which I believe is a legitimate configuration,
especially since distros so build kernels with lots of modules.
I'll double check on this though in case I've made a mistake.

> 
> Please notice that the object sizes are significantly smaller below:
> 
> So with an x86-64 defconfig and this compilation unit enabled with
> CONFIG_OF enabled and CONFIG_DRM_TOSHIBA_TC358767=y, with gcc 10.3
> and this change the object size actually increases a bit.
> 
> $ size drivers/gpu/drm/bridge/tc358767.o*
>    text	   data	    bss	    dec	    hex	filename
>   13554	    268	      1	  13823	   35ff	drivers/gpu/drm/bridge/tc358767.o.new
>   13548	    268	      1	  13817	   35f9	drivers/gpu/drm/bridge/tc358767.o.old>
> objdump -h shows these differences:
> 
> .old:
>   0 .text         00001e1f  0000000000000000  0000000000000000  00000040  2**4
>                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
> [...]
>  14 .rodata       000005ae  0000000000000000  0000000000000000  000046e0  2**5
>                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
> 
> .new:
>   0 .text         00001e05  0000000000000000  0000000000000000  00000040  2**4
>                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
> [...]
>  11 .rodata       000005ce  0000000000000000  0000000000000000  00004600  2**5
>                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA

ACK. Understood.  Even so, it still makes sense for these kind of
janitorial changes as it makes sense to constify arrays when they are
read-only and making them static is sensible for const data.

> 
> cheers, Joe
> 

Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller

[Joe Perches]

On Thu, 2021-08-19 at 15:51 +0100, Colin Ian King wrote:

> it still makes sense for these kind of
> janitorial changes as it makes sense to constify arrays when they are
> read-only and making them static is sensible for const data.

I'm not disagreeing. Marking unmodifiable arrays as const is generally
useful for readers.  Decent compilers though can _mostly_ determine
whether or not an array is used as const and whether the array can be
placed in a readonly section and is not required to be in a writable one.

But the object sizes deltas you show with an allmodconfig are misleading.
At a minimum I think you should show the output sizes as allmodconfig.

Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller

[Joe Perches]

On Thu, 2021-08-19 at 15:51 +0100, Colin Ian King wrote:

> it still makes sense for these kind of
> janitorial changes as it makes sense to constify arrays when they are
> read-only and making them static is sensible for const data.

I'm not disagreeing. Marking unmodifiable arrays as const is generally
useful for readers.  Decent compilers though can _mostly_ determine
whether or not an array is used as const and whether the array can be
placed in a readonly section and is not required to be in a writable one.

But the object sizes deltas you show with an allmodconfig are misleading.
At a minimum I think you should show the output sizes as allmodconfig.

Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller

[Sam Ravnborg]

Hi Colin,

On Thu, Aug 19, 2021 at 02:38:39PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Don't populate the array ext_div on the stack but instead it
> static const. Makes the object code smaller by 118 bytes:
> 
> Before:
>    text    data    bss     dec    hex filename
>   39449   17500    128   57077   def5 ./drivers/gpu/drm/bridge/tc358767.o
> 
> After:
>    text    data    bss     dec    hex filename
>   39235   17596    128   56959   de7f ./drivers/gpu/drm/bridge/tc358767.o
> 
> (gcc version 10.3.0)

IMO a better argument is that this change prevents any accidental
changes and it align with how we define arrays in many other places.
The compiler may produce a smaller binary but that is just a side-effect
in this case.

	Sam

> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 23a6f90b694b..599c23759400 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -468,7 +468,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
>  	int div, best_div = 1;
>  	int mul, best_mul = 1;
>  	int delta, best_delta;
> -	int ext_div[] = {1, 2, 3, 5, 7};
> +	static const int ext_div[] = {1, 2, 3, 5, 7};
>  	int best_pixelclock = 0;
>  	int vco_hi = 0;
>  	u32 pxl_pllparam;
> -- 
> 2.32.0