RFC: AVR build failure & hot/cold partitioning

[Richard Sandiford <richard at codesourcery dot com>]

One of the things I found hardest to convert in the section rewrite
patch was the hot/cold partitioning code.  In the end, I think I took
the "last_text_section" variable too literally.  The old section code
would set last_text_section in the following places:

  - assemble_start_function(), to no_section.

  - text_section(), to in_text_section.

  - unlikely_text_section(), to in_unlikely_executed_text.

  - named_section(), in:

      if (in_text_section () || in_unlikely_text_section ())
        {
          last_text_section = in_section;
          last_text_section_name = name;
        }
      
    (The use of in_text_section() seems redundant.  We only get here
    with in_section == in_named, because that's what named_section is
    supposed to achieve.)

  - the darwin EXTRA_SECTIONS functions, if the new section was one
    of in_text_coal, in_text_unlikely or in_text_unlikely_coal

I changed this so that it was set twice:

  - assemble_start_function(), to NULL.

  - select_section, to the new section if it had SECTION_CODE set.

This isn't safe because we can set last_text_section for sections that
we previously didn't.  An example is the avr progmem_section, which
is a code section if !AVR_MEGA.  After my patch, there are cases where
we can wrongly assume that progmem_section rather than text_section is
being used for the current function's code.  This causes a build failure
for unwind-dw2-fde.c.  (Unfortunately, the problem didn't show up in the
cases covered by my assembly-comparison tests.)

As I understand it:

  (a) All uses of last_text_section are trying to decide whether we
      are in hot or cold code.

  (b) The setting for the beginning of a function is controlled by
      first_function_block_is_cold.

  (c) Thereafter, the setting changes only in response to a
      NOTE_INSN_SWITCH_TEXT_SECTIONS.

Is that right?  If so, would it be OK to replace last_text_section
with a simple boolean flag that is set at the beginning of a function
and then only changed by NOTE_INSN_SWITCH_TEXT_SECTIONS?  The patch
below implements this.

A couple of things I don't understand:

  (1) The NOTE_INSN_SWITCH_TEXT_SECTIONS code seems to assume a choice
      between the (hot) text_section and a function-specific cold section.
      However, the darwin code allowed text_coal to be used as the hot
      section.  (See the old definition of EXTRA_SECTIONS.)

  (2) function_section has two cases.  The USE_SELECT_SECTION_FOR_FUNCTIONS
      one always uses first_function_block_is_cold, even though the DECL
      argument could in theory be for any function, not just the current one.
      The (usual) !USE_SELECT_SECTION_FOR_FUNCTIONS case ignores
      first_function_block_is_cold altogether.

      Is this deliberate?  How does the normal code handle functions that
      start in a cold section?

The patch changes (1) so that we always trust current_function_section.
This seems more correct in principle and falls out in the wash.

I didn't want to change (2) here, but at the same time, I didn't
want to rely on the current behaviour of function_section for
!USE_SELECT_SECTION_FOR_FUNCTIONS.  The patch therefore introduces
a new function called hot_section_function and uses it to implement
both function_section and current_function_section.

The patch has been bootstrapped on i686-pc-linux-gnu, but I haven't
yet finished building the target libraries or running regression tests.
I'm posting it now because of the AVR build problem and because of the
doubts expressed above.  Does it look vaguely sane?  If so, could someone
with access to Darwin please give it a spin?

Richard


	* final.c (final_scan_insn): Flip in_cold_section_p when changing
	between the hot and cold sections.  Use current_function_section ()
	to get the new section.
	* dwarf2out.c (output_line_info): Use in_cold_section_p to
	determine whether we are assembling hot or cold code.
	(secname_for_decl, dwarf2out_var_location): Likewise.
	(dwarf2out_init, dwarf2out_finish): Use switch_to_section.
	* varasm.c (last_text_section): Delete.
	(in_cold_section_p): New variable.
	(hot_function_section): New function.
	(current_function_section): Pass in_cold_section_p as the reloc
	argument to select_section.  Use it to decide between
	unlikely_function_section () and hot_function_section ().
	(assemble_start_function): Use switch_to_section.  Set
	in_cold_section_p instead of last_text_section.
	(assemble_end_function): Use switch_to_section.
	(switch_to_section): Don't set last_text_section.
	* config/darwin/darwin.c (machopic_select_section): Trust the reloc
	argument to make the right choice between hot and cold sections.

Index: gcc/final.c
===================================================================
--- gcc/final.c	(revision 108162)
+++ gcc/final.c	(working copy)
@@ -1708,22 +1708,9 @@ final_scan_insn (rtx insn, FILE *file, i
 	  break;
 
 	case NOTE_INSN_SWITCH_TEXT_SECTIONS:
-	  
-	  /* The presence of this note indicates that this basic block
-	     belongs in the "cold" section of the .o file.  If we are
-	     not already writing to the cold section we need to change
-	     to it.  */
-
-	  if (last_text_section == text_section)
-	    {
-	      (*debug_hooks->switch_text_section) ();
-	      switch_to_section (unlikely_text_section ());
-	    }
-	  else
-	    {
-	      (*debug_hooks->switch_text_section) ();
-	      switch_to_section (text_section);
-	    }
+	  in_cold_section_p = !in_cold_section_p;
+	  (*debug_hooks->switch_text_section) ();
+	  switch_to_section (current_function_section ());
 	  break;
 	  
 	case NOTE_INSN_BASIC_BLOCK:
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 108162)
+++ gcc/dwarf2out.c	(working copy)
@@ -7840,7 +7840,7 @@ output_line_info (void)
   current_file = 1;
   current_line = 1;
 
-  if (cfun && unlikely_text_section_p (last_text_section))
+  if (cfun && in_cold_section_p)
     strcpy (prev_line_label, cfun->cold_section_label);
   else
     strcpy (prev_line_label, text_section_label);
@@ -10185,7 +10185,7 @@ secname_for_decl (tree decl)
       tree sectree = DECL_SECTION_NAME (current_function_decl);
       secname = TREE_STRING_POINTER (sectree);
     }
-  else if (cfun && unlikely_text_section_p (last_text_section))
+  else if (cfun && in_cold_section_p)
     secname = cfun->cold_section_label;
   else
     secname = text_section_label;
@@ -13555,7 +13555,7 @@ dwarf2out_var_location (rtx loc_note)
   newloc->var_loc_note = loc_note;
   newloc->next = NULL;
 
-  if (cfun && unlikely_text_section_p (last_text_section))
+  if (cfun && in_cold_section_p)
     newloc->section_label = cfun->cold_section_label;
   else
     newloc->section_label = text_section_label;
@@ -13844,7 +13844,7 @@ dwarf2out_init (const char *filename ATT
   ASM_OUTPUT_LABEL (asm_out_file, text_section_label);
   if (flag_reorder_blocks_and_partition)
     {
-      unlikely_text_section ();
+      switch_to_section (unlikely_text_section ());
       ASM_OUTPUT_LABEL (asm_out_file, cold_text_section_label);
     }
 }
@@ -14180,7 +14180,7 @@ dwarf2out_finish (const char *filename)
   targetm.asm_out.internal_label (asm_out_file, TEXT_END_LABEL, 0);
   if (flag_reorder_blocks_and_partition)
     {
-      unlikely_text_section ();
+      switch_to_section (unlikely_text_section ());
       targetm.asm_out.internal_label (asm_out_file, COLD_END_LABEL, 0);
     }
 
Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(revision 108162)
+++ gcc/varasm.c	(working copy)
@@ -167,8 +167,9 @@ static void mark_weak (tree);
    been selected or if we lose track of what the current section is.  */
 section *in_section;
 
-/* The last text section used by asm_out_file.  */
-section *last_text_section;
+/* True if code for the current function is currently being directed
+   at the cold section.  */
+bool in_cold_section_p;
 
 /* A linked list of all the unnamed sections.  */
 static GTY(()) section *unnamed_sections;
@@ -410,6 +411,24 @@ asm_output_aligned_bss (FILE *file, tree
 
 #endif /* BSS_SECTION_ASM_OP */
 
+/* Return the hot section for function DECL.  Return text_section for
+   null DECLs.  */
+
+static section *
+hot_function_section (tree decl)
+{
+#ifdef USE_SELECT_SECTION_FOR_FUNCTIONS
+  return targetm.asm_out.select_section (decl, 0, DECL_ALIGN (decl));
+#else
+  if (decl != NULL_TREE
+      && DECL_SECTION_NAME (decl) != NULL_TREE
+      && targetm.have_named_sections)
+    return get_named_section (decl, NULL, 0);
+  else
+    return text_section;
+#endif
+}
+
 /* Return the section for function DECL.
 
    If DECL is NULL_TREE, return the text section.  We can be passed
@@ -426,12 +445,7 @@ function_section (tree decl)
 #ifdef USE_SELECT_SECTION_FOR_FUNCTIONS
   return targetm.asm_out.select_section (decl, reloc, DECL_ALIGN (decl));
 #else
-  if (decl != NULL_TREE
-      && DECL_SECTION_NAME (decl) != NULL_TREE
-      && targetm.have_named_sections)
-    return get_named_section (decl, NULL, 0);
-  else
-    return text_section;
+  return hot_function_section (decl);
 #endif
 }
 
@@ -439,17 +453,13 @@ function_section (tree decl)
 current_function_section (void)
 {
 #ifdef USE_SELECT_SECTION_FOR_FUNCTIONS
-  int reloc = 0; 
-
-  if (unlikely_text_section_p (last_text_section))
-    reloc = 1;
- 
-  return targetm.asm_out.select_section (current_function_decl, reloc,
+  return targetm.asm_out.select_section (current_function_decl,
+					 in_cold_section_p,
 					 DECL_ALIGN (current_function_decl));
 #else
-  if (last_text_section)
-    return last_text_section;
-  return function_section (current_function_decl);
+  return (in_cold_section_p
+	  ? unlikely_text_section ()
+	  : hot_function_section (current_function_decl));
 #endif
 }
 
@@ -1082,7 +1092,7 @@ assemble_start_function (tree decl, cons
 
   if (flag_reorder_blocks_and_partition)
     {
-      unlikely_text_section ();
+      switch_to_section (unlikely_text_section ());
       assemble_align (FUNCTION_BOUNDARY);
       ASM_OUTPUT_LABEL (asm_out_file, cfun->cold_section_label);
 
@@ -1114,7 +1124,7 @@ assemble_start_function (tree decl, cons
 	first_function_block_is_cold = true;
     }
 
-  last_text_section = NULL;
+  in_cold_section_p = first_function_block_is_cold;
 
   /* Switch to the correct text section for the start of the function.  */
 
@@ -1199,7 +1209,7 @@ assemble_end_function (tree decl, const 
       section *save_text_section;
 
       save_text_section = in_section;
-      unlikely_text_section ();
+      switch_to_section (unlikely_text_section ());
       ASM_OUTPUT_LABEL (asm_out_file, cfun->cold_section_end_label);
       if (first_function_block_is_cold)
 	switch_to_section (text_section);
@@ -5599,11 +5609,7 @@ switch_to_section (section *new_section)
   if (new_section->common.flags & SECTION_FORGET)
     in_section = NULL;
   else
-    {
-      in_section = new_section;
-      if (new_section->common.flags & SECTION_CODE)
-	last_text_section = in_section;
-    }
+    in_section = new_section;
 
   if (new_section->common.flags & SECTION_NAMED)
     {
Index: gcc/output.h
===================================================================
--- gcc/output.h	(revision 108162)
+++ gcc/output.h	(working copy)
@@ -507,7 +507,7 @@ extern GTY(()) section *exception_sectio
 extern GTY(()) section *eh_frame_section;
 
 extern GTY(()) section *in_section;
-extern GTY(()) section *last_text_section;
+extern GTY(()) bool in_cold_section_p;
 
 extern section *get_unnamed_section (unsigned int, void (*) (const void *),
 				     const void *);
Index: gcc/config/darwin.c
===================================================================
--- gcc/config/darwin.c	(revision 108162)
+++ gcc/config/darwin.c	(working copy)
@@ -1093,9 +1093,7 @@ machopic_select_section (tree exp, int r
 
   if (TREE_CODE (exp) == FUNCTION_DECL)
     {
-      if (reloc == 1
-	  || unlikely_text_section_p (last_text_section)
-	  || last_text_section == text_unlikely_coal_section)
+      if (reloc == 1)
 	base_section = (weak_p
 			? text_unlikely_coal_section
 			: unlikely_text_section ());