Message ID | 4DAC72C5.1090004@gjlay.de |
---|---|
State | New |
Headers | show |
Hi. > > +/* To track if code will use .bss and/or .data */ > +static int avr_need_clear_bss_p = 0; > +static int avr_need_copy_data_p = 0; Change type avr_need_clear_bss_p and avr_need_copy_data_p vars to bool. > > -#define ASM_OUTPUT_COMMON(STREAM, NAME, SIZE, ROUNDED) \ > -do { \ > - fputs ("\t.comm ", (STREAM)); \ > - assemble_name ((STREAM), (NAME)); \ > - fprintf ((STREAM), ",%lu,1\n", (unsigned long)(SIZE)); \ > -} while (0) > +#define ASM_OUTPUT_COMMON(STREAM, NAME, SIZE, ROUNDED) \ > + avr_asm_output_common (STREAM, NAME, SIZE, ROUNDED) > Use ASM_OUTPUT_ALIGNED_DECL_COMMON macro instead of ASM_OUTPUT_COMMON. > > -#define ASM_OUTPUT_LOCAL(STREAM, NAME, SIZE, ROUNDED) \ > -do { \ > - fputs ("\t.lcomm ", (STREAM)); \ > - assemble_name ((STREAM), (NAME)); \ > - fprintf ((STREAM), ",%d\n", (int)(SIZE)); \ > -} while (0) > +#define ASM_OUTPUT_LOCAL(STREAM, NAME, SIZE, ROUNDED) \ > + avr_asm_output_local (STREAM, NAME, SIZE, ROUNDED) > Use ASM_OUTPUT_ALIGNED_DECL_LOCAL macro instead of ASM_OUTPUT_LOCAL. Anatoly.
Georg-Johann Lay schrieb: > This is a port of an old patch of mine that got integrated in some > avr-gcc distributions. > > Linking against __do_copy_data resp. __do_clear_bss is only needed if > there is actually stuff in .[ro]data resp. .bss. This saves some space > on tiny targets. > > Regression-tested for C > > Johann > > 2011-04-18 Georg-Johann Lay <avr@gjlay.de> > > PR target/18145 > > * config/avr/avr.h (TARGET_ASM_INIT_SECTIONS): Move to > config/avr/avr.c > (TARGET_ASM_NAMED_SECTION): Change to avr_asm_named_section. > (ASM_OUTPUT_COMMON): Forward to avr_asm_output_common. > (ASM_OUTPUT_LOCAL): Forward to avr_asm_output_local. > > * config/avr/avr-protos.h (avr_asm_output_common, > avr_asm_output_local): New prototypes. > > * config/avr/avr.c . > (avr_asm_named_section, avr_asm_output_local, > avr_asm_output_common, avr_output_data_section_asm_op, > avr_output_bss_section_asm_op): New functions to update... > (avr_need_clear_bss_p, avr_need_copy_data_p): ...thise new variables. > (TARGET_ASM_INIT_SECTIONS): Overwrite section callbacks for > data_section, bss_section. > (avr_file_start): Move output of __do_copy_data, __do_clear_bss > from here to... > (avr_file_end): ...here. Fixed ChangeLog PR target/18145 * config/avr/avr.h (TARGET_ASM_INIT_SECTIONS): Delete. (TARGET_ASM_NAMED_SECTION): Change to avr_asm_named_section. (ASM_OUTPUT_COMMON): Forward to avr_asm_output_common. (ASM_OUTPUT_LOCAL): Forward to avr_asm_output_local. * config/avr/avr-protos.h (avr_asm_output_common, avr_asm_output_local): New prototypes. * config/avr/avr.c (avr_asm_named_section, avr_asm_output_local, avr_asm_output_common, avr_output_data_section_asm_op, avr_output_bss_section_asm_op): New functions to update... (avr_need_clear_bss_p, avr_need_copy_data_p): ...thise new variables. (TARGET_ASM_INIT_SECTIONS): Define. (avr_asm_init_sections): Overwrite section callbacks for data_section, bss_section. (avr_file_start): Move output of __do_copy_data, __do_clear_bss from here to... (avr_file_end): ...here.
On 04/18/2011 10:20 AM, Georg-Johann Lay wrote: > +avr_asm_named_section (const char *name, unsigned int flags, tree decl) > +{ > + if (!avr_need_copy_data_p) > + avr_need_copy_data_p = ((0 == strncmp (name, ".data", 5) > + || 0 == strncmp (name, ".rodata", 7) > + || 0 == strncmp (name, ".gnu.linkonce.", 14))); > + > + if (!avr_need_clear_bss_p) > + avr_need_clear_bss_p = (0 == strncmp (name, ".bss", 4)); Have a look at FLAGS. I expect that you can reference those to categorize the data rather than hard-coding the section names. In particular I believe your ".gnu.linkonce" test is wrong. It's not clear to me what you're looking for wrt "data". Perhaps what you want is if (flags & (SECTION_DEBUG | SECTION_CODE)) ; else if (flags & SECTION_BSS) avr_need_clear_bss_p = true; else avr_need_copy_data_p = true; r~
> -----Original Message----- > From: Richard Henderson [mailto:rth@redhat.com] > Sent: Tuesday, April 19, 2011 1:31 PM > To: Georg-Johann Lay > Cc: gcc-patches@gcc.gnu.org; Weddington, Eric; Denis Chertykov; Anatoly > Sokolov > Subject: Re: [Patch,AVR]: PR18145: do_copy_data & do_clear_bss only if > needed > > On 04/18/2011 10:20 AM, Georg-Johann Lay wrote: > > +avr_asm_named_section (const char *name, unsigned int flags, tree decl) > > +{ > > + if (!avr_need_copy_data_p) > > + avr_need_copy_data_p = ((0 == strncmp (name, ".data", 5) > > + || 0 == strncmp (name, ".rodata", 7) > > + || 0 == strncmp (name, ".gnu.linkonce.", > 14))); > > + > > + if (!avr_need_clear_bss_p) > > + avr_need_clear_bss_p = (0 == strncmp (name, ".bss", 4)); > > Have a look at FLAGS. I expect that you can reference those to > categorize the data rather than hard-coding the section names. > In particular I believe your ".gnu.linkonce" test is wrong. > > It's not clear to me what you're looking for wrt "data". We're looking for the existence of global initialized data variables. We have 2 small subroutines in our libgcc, one to set everything in .bss to zero, and another to copy the initializations from .text (in flash) to the RAM based variables in .data. These subroutines have always been included in the startup code whether they were needed or not. The purpose of this patch is to check whether these subroutines are actually needed or not (i.e. if anything actually exists in .bss, or in .data). If either of these sections are actually empty, then we don't want to link in the corresponding subroutine, which helps us save some code space on small devices. AFAICT, I've never seen the avr target generate anything in .rodata and .gnu.linkonce sections. I think these might be holdovers from some other target code and/or there for some completeness reason, I'm not really sure. Eric Weddington
Weddington, Eric schrieb: > >>-----Original Message----- >>From: Richard Henderson [mailto:rth@redhat.com] >>Sent: Tuesday, April 19, 2011 1:31 PM >>To: Georg-Johann Lay >>Cc: gcc-patches@gcc.gnu.org; Weddington, Eric; Denis Chertykov; Anatoly >>Sokolov >>Subject: Re: [Patch,AVR]: PR18145: do_copy_data & do_clear_bss only if >>needed >> >>On 04/18/2011 10:20 AM, Georg-Johann Lay wrote: >> >>>+avr_asm_named_section (const char *name, unsigned int flags, tree decl) >>>+{ >>>+ if (!avr_need_copy_data_p) >>>+ avr_need_copy_data_p = ((0 == strncmp (name, ".data", 5) >>>+ || 0 == strncmp (name, ".rodata", 7) >>>+ || 0 == strncmp (name, ".gnu.linkonce.", >> >>14))); >> >>>+ >>>+ if (!avr_need_clear_bss_p) >>>+ avr_need_clear_bss_p = (0 == strncmp (name, ".bss", 4)); >> >>Have a look at FLAGS. I expect that you can reference those to >>categorize the data rather than hard-coding the section names. >>In particular I believe your ".gnu.linkonce" test is wrong. >> >>It's not clear to me what you're looking for wrt "data". > > > We're looking for the existence of global initialized data variables. > > We have 2 small subroutines in our libgcc, one to set everything in > .bss to zero, and another to copy the initializations from .text (in > flash) to the RAM based variables in .data. These subroutines have > always been included in the startup code whether they were needed or > not. The purpose of this patch is to check whether these subroutines > are actually needed or not (i.e. if anything actually exists in .bss, > or in .data). If either of these sections are actually empty, then we > don't want to link in the corresponding subroutine, which helps us > save some code space on small devices. > > AFAICT, I've never seen the avr target generate anything in .rodata > and .gnu.linkonce sections. I think these might be holdovers from > some other target code and/or there for some completeness reason, I'm > not really sure. When I wrote this patch I looked at the default linker script to see what goes into .data resp .bss; the hard-coded section maned reflect that. For the linkonce stuff I found no explanation (grepping the web yields bunch of questions from people having trouble with it) and in the gcc wiki you find nothing (except you have already a link to what you seek). The .gnu.linkonce is an overestimation, .gnu.linkonce.d. is perhaps better, but exceptions are not supported on avr, anyway. So an overestimation won't hurt. Then I wonder where .noinit, .progmem.data, .progmem.gcc_sw_table would go? Moreover, with checking section names it might be easier for users that want some private sections treated as cleared by __do_clear_bss or initialized by __do_copy_data. AFAIK the section attribute does not allow to set section flags? Eric, you don't see .rodata because of avr.c:avr_asm_init_sections readonly_data_section = data_section; To see .rodata give -fdata-sections for const not in progmem. The only thing I am a bit uncomfortable with is that there is no command line option to unconditionally reference the __do's. Johann
Georg-Johann Lay schrieb: > When I wrote this patch I looked at the default linker script to see > what goes into .data resp .bss; the hard-coded section maned reflect . names > that. For the linkonce stuff I found no explanation (grepping the web > yields bunch of questions from people having trouble with it) and in the > gcc wiki you find nothing (except you have already a link to what you > seek). > > The .gnu.linkonce is an overestimation, .gnu.linkonce.d. is perhaps > better, but exceptions are not supported on avr, anyway. So an > overestimation won't hurt. > > Then I wonder where .noinit, .progmem.data, .progmem.gcc_sw_table > would go? I mean what flags they get. .init* gets SECTION_BSS in avr_section_type_flags, but there is also a hard-coded section name. Moreover, there is a discrepancy between C and C++, see PR34734. In C++, avr_section_type_flags sees no DECL_INITIAL (decl) even if one is present. I read tree stuff and they make no difference between C/C++ and promise DECL_INITIAL is there. Perhaps it's because of different parsers? avr_section_type_flags could attach (target specific) flags. However, checking section names explicitely appeared more robust to me, especially because PR34734 indicates that not all information is readily available. Moreover, this patch is integrated in some avr-gcc distributions and has experienced some real world testing. I didn't hear complaints, but it's likely that I missed them because I am just volonteering and have long times of inactivity because of technical reasons then and when. Eric has definetely a better oversight. Johann
> -----Original Message----- > From: Georg-Johann Lay [mailto:avr@gjlay.de] > Sent: Tuesday, April 19, 2011 2:13 PM > To: Weddington, Eric > Cc: Richard Henderson; gcc-patches@gcc.gnu.org; Denis Chertykov; Anatoly > Sokolov > Subject: Re: [Patch,AVR]: PR18145: do_copy_data & do_clear_bss only if > needed > > Weddington, Eric schrieb: > > > Eric, you don't see .rodata because of avr.c:avr_asm_init_sections > readonly_data_section = data_section; > To see .rodata give -fdata-sections for const not in progmem. Ah, ok. Hardly any real world user specifies -fdata-sections. They might do -ffunction-sections though. > The only thing I am a bit uncomfortable with is that there is no command > line option to unconditionally reference the __do's. Why would a user want to do that? Unless of course your patch is buggy. ;-) Eric Weddington
Index: config/avr/avr-protos.h =================================================================== --- config/avr/avr-protos.h (Revision 172597) +++ config/avr/avr-protos.h (Arbeitskopie) @@ -34,6 +34,8 @@ extern void gas_output_limited_string (F extern void gas_output_ascii (FILE *file, const char *str, size_t length); extern int avr_hard_regno_rename_ok (unsigned int, unsigned int); extern rtx avr_return_addr_rtx (int count, rtx tem); +extern void avr_asm_output_common (FILE*, const char*, unsigned HOST_WIDE_INT, unsigned HOST_WIDE_INT); +extern void avr_asm_output_local (FILE*, const char*, unsigned HOST_WIDE_INT, unsigned HOST_WIDE_INT); #ifdef TREE_CODE extern void asm_output_external (FILE *file, tree decl, char *name); Index: config/avr/avr.c =================================================================== --- config/avr/avr.c (Revision 172597) +++ config/avr/avr.c (Arbeitskopie) @@ -108,6 +108,7 @@ static void avr_function_arg_advance (CU const_tree, bool); static void avr_help (void); static bool avr_function_ok_for_sibcall (tree, tree); +static void avr_asm_named_section (const char *name, unsigned int flags, tree decl); /* Allocate registers from r25 to r8 for parameters for function calls. */ #define FIRST_CUM_REG 26 @@ -132,6 +133,10 @@ const struct mcu_type_s *avr_current_dev section *progmem_section; +/* To track if code will use .bss and/or .data */ +static int avr_need_clear_bss_p = 0; +static int avr_need_copy_data_p = 0; + /* AVR attributes. */ static const struct attribute_spec avr_attribute_table[] = { @@ -197,6 +202,10 @@ static const struct default_options avr_ #define TARGET_INSERT_ATTRIBUTES avr_insert_attributes #undef TARGET_SECTION_TYPE_FLAGS #define TARGET_SECTION_TYPE_FLAGS avr_section_type_flags + +#undef TARGET_ASM_INIT_SECTIONS +#define TARGET_ASM_INIT_SECTIONS avr_asm_init_sections + #undef TARGET_REGISTER_MOVE_COST #define TARGET_REGISTER_MOVE_COST avr_register_move_cost #undef TARGET_MEMORY_MOVE_COST @@ -5190,6 +5199,65 @@ avr_output_progmem_section_asm_op (const fprintf (asm_out_file, "\t.p2align 1\n"); } + +/* Implement `ASM_OUTPUT_LOCAL' */ +/* Track need of __do_clear_bss */ + +void +avr_asm_output_local (FILE * stream, + const char *name, + unsigned HOST_WIDE_INT size, + unsigned HOST_WIDE_INT rounded ATTRIBUTE_UNUSED) +{ + avr_need_clear_bss_p = 1; + + fputs ("\t.lcomm ", stream); + assemble_name (stream, name); + fprintf (stream, ",%d\n", (int) size); +} + + +/* Implement `ASM_OUTPUT_COMMON' */ +/* Track need of __do_clear_bss */ + +void +avr_asm_output_common (FILE * stream, + const char *name, + unsigned HOST_WIDE_INT size, + unsigned HOST_WIDE_INT rounded ATTRIBUTE_UNUSED) +{ + avr_need_clear_bss_p = 1; + + fputs ("\t.comm ", stream); + assemble_name (stream, name); + fprintf (stream, ",%lu,1\n", (unsigned long) size); +} + + +/* Unnamed section callback for data_section + to track need of __do_copy_data */ + +static void +avr_output_data_section_asm_op (const void *data) +{ + avr_need_copy_data_p = 1; + /* Dispatch to default */ + output_section_asm_op (data); +} + + +/* Unnamed section callback for bss_section + to to track need of __do_clear_bss */ + +static void +avr_output_bss_section_asm_op (const void *data) +{ + avr_need_clear_bss_p = 1; + /* Dispatch to default */ + output_section_asm_op (data); +} + + /* Implement TARGET_ASM_INIT_SECTIONS. */ static void @@ -5199,6 +5267,27 @@ avr_asm_init_sections (void) avr_output_progmem_section_asm_op, NULL); readonly_data_section = data_section; + + data_section->unnamed.callback = avr_output_data_section_asm_op; + bss_section->unnamed.callback = avr_output_bss_section_asm_op; +} + + +/* Implement `TARGET_ASM_NAMED_SECTION' */ +/* Track need of __do_clear_bss, __do_copy_data for named sections */ + +void +avr_asm_named_section (const char *name, unsigned int flags, tree decl) +{ + if (!avr_need_copy_data_p) + avr_need_copy_data_p = ((0 == strncmp (name, ".data", 5) + || 0 == strncmp (name, ".rodata", 7) + || 0 == strncmp (name, ".gnu.linkonce.", 14))); + + if (!avr_need_clear_bss_p) + avr_need_clear_bss_p = (0 == strncmp (name, ".bss", 4)); + + default_elf_asm_named_section (name, flags, decl); } static unsigned int @@ -5237,12 +5326,6 @@ avr_file_start (void) fputs ("__tmp_reg__ = 0\n" "__zero_reg__ = 1\n", asm_out_file); - - /* FIXME: output these only if there is anything in the .data / .bss - sections - some code size could be saved by not linking in the - initialization code from libgcc if one or both sections are empty. */ - fputs ("\t.global __do_copy_data\n", asm_out_file); - fputs ("\t.global __do_clear_bss\n", asm_out_file); } /* Outputs to the stdio stream FILE some @@ -5251,6 +5334,17 @@ avr_file_start (void) static void avr_file_end (void) { + /* Output these only if there is anything in the + .data* / .rodata* / .gnu.linkonce.* resp. .bss* + input section(s) - some code size can be saved by not + linking in the initialization code from libgcc if resp. + sections are empty. */ + + if (avr_need_copy_data_p) + fputs (".global __do_copy_data\n", asm_out_file); + + if (avr_need_clear_bss_p) + fputs (".global __do_clear_bss\n", asm_out_file); } /* Choose the order in which to allocate hard registers for Index: config/avr/avr.h =================================================================== --- config/avr/avr.h (Revision 172597) +++ config/avr/avr.h (Arbeitskopie) @@ -460,29 +460,20 @@ do { \ #define ASM_APP_OFF "/* #NOAPP */\n" /* Switch into a generic section. */ -#define TARGET_ASM_NAMED_SECTION default_elf_asm_named_section -#define TARGET_ASM_INIT_SECTIONS avr_asm_init_sections +#define TARGET_ASM_NAMED_SECTION avr_asm_named_section #define ASM_OUTPUT_ASCII(FILE, P, SIZE) gas_output_ascii (FILE,P,SIZE) #define IS_ASM_LOGICAL_LINE_SEPARATOR(C, STR) ((C) == '\n' || ((C) == '$')) -#define ASM_OUTPUT_COMMON(STREAM, NAME, SIZE, ROUNDED) \ -do { \ - fputs ("\t.comm ", (STREAM)); \ - assemble_name ((STREAM), (NAME)); \ - fprintf ((STREAM), ",%lu,1\n", (unsigned long)(SIZE)); \ -} while (0) +#define ASM_OUTPUT_COMMON(STREAM, NAME, SIZE, ROUNDED) \ + avr_asm_output_common (STREAM, NAME, SIZE, ROUNDED) #define ASM_OUTPUT_ALIGNED_BSS(FILE, DECL, NAME, SIZE, ALIGN) \ asm_output_aligned_bss (FILE, DECL, NAME, SIZE, ALIGN) -#define ASM_OUTPUT_LOCAL(STREAM, NAME, SIZE, ROUNDED) \ -do { \ - fputs ("\t.lcomm ", (STREAM)); \ - assemble_name ((STREAM), (NAME)); \ - fprintf ((STREAM), ",%d\n", (int)(SIZE)); \ -} while (0) +#define ASM_OUTPUT_LOCAL(STREAM, NAME, SIZE, ROUNDED) \ + avr_asm_output_local (STREAM, NAME, SIZE, ROUNDED) #undef TYPE_ASM_OP #undef SIZE_ASM_OP