diff mbox

[AVR] : PR18145: do_copy_data & do_clear_bss only if needed

Message ID 4DAC72C5.1090004@gjlay.de
State New
Headers show

Commit Message

Georg-Johann Lay April 18, 2011, 5:20 p.m. UTC
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.

Comments

Anatoly Sokolov April 18, 2011, 5:33 p.m. UTC | #1
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 April 18, 2011, 5:36 p.m. UTC | #2
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.
Richard Henderson April 19, 2011, 7:30 p.m. UTC | #3
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~
Weddington, Eric April 19, 2011, 7:47 p.m. UTC | #4
> -----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
Georg-Johann Lay April 19, 2011, 8:13 p.m. UTC | #5
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 April 19, 2011, 8:33 p.m. UTC | #6
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
Weddington, Eric April 19, 2011, 9:43 p.m. UTC | #7
> -----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
diff mbox

Patch

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