| Message ID | 20210819133839.10745-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge/tc358767: make the array ext_div static const, makes object smaller | expand |
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;
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; > >
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
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 >
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.
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
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;