Re: Group static constructors and destructors in specific subsections, take 2

[Richard Henderson <rth at redhat dot com>]

On 11/09/2010 08:24 PM, Jan Hubicka wrote:
> Index: config/ia64/hpux.h
> ===================================================================
> --- config/ia64/hpux.h	(revision 166490)
> +++ config/ia64/hpux.h	(working copy)
> @@ -221,8 +221,4 @@ do {								\
>     it is fixed, prevent code from being put into .text.unlikely or
>     .text.hot.  */
>  
> -#undef UNLIKELY_EXECUTED_TEXT_SECTION_NAME
> -#define UNLIKELY_EXECUTED_TEXT_SECTION_NAME ".text"
> -
> -#undef HOT_TEXT_SECTION_NAME
> -#define HOT_TEXT_SECTION_NAME ".text"
> +#define TARGET_ASM_FUNCTION_SECTOIN NULL

Typo.  But I'd rather avoid a check for NULL that affects only one target.
Better to define a function that does what you want.

>  #ifdef USE_SELECT_SECTION_FOR_FUNCTIONS
>    if (decl != NULL_TREE
>        && DECL_SECTION_NAME (decl) != NULL_TREE)
> -    return reloc ? unlikely_text_section ()
> -		 : get_named_section (decl, NULL, 0);
> +    {
> +      if (targetm.asm_out.function_section)
> +	section = targetm.asm_out.function_section (decl, freq,
> +						    startup, exit);
> +      if (section)
> +	return section;
> +      return get_named_section (decl, NULL, 0);
> +    }
>    else
> -    return targetm.asm_out.select_section (decl, reloc, DECL_ALIGN (decl));
> +    return targetm.asm_out.select_section
> +	    (decl, freq == NODE_FREQUENCY_UNLIKELY_EXECUTED,
> +	     DECL_ALIGN (decl));
>  #else
> -  return reloc ? unlikely_text_section () : hot_function_section (decl);
> +  if (targetm.asm_out.function_section)
> +    section = targetm.asm_out.function_section (decl, freq, startup, exit);
> +  if (section)
> +    return section;
> +  return hot_function_section (decl);
>  #endif

Honestly, adding the function_section hook is an excellent opportunity
to get rid of the USE_SELECT_SECTION_FOR_FUNCTIONS macro, used only by
darwin and the MEP port.

I think this whole section should boil down to 

  return targetm.asm_out.function_section (decl, freq, startup, exit);

with the associated ugliness above pushed into various versions of the
function_section hook.

Also, I think hot_function_section should be merged here, so that we're
properly checking for DECL_SECTION_NAME before deferring to the new
function_section hook.  Which means that the hook will either gen up
a new section name based on (freq,startup,exit), or return text_section.

> +section *
> +named_text_subsection (tree decl,
> +		       const char *text_section_name,
> +		       const char *named_section_suffix)

I don't understand why you're bothering to hash the section here, when
that would also be done inside get_section.  Seems like it'd be just as
easy to concat the section name you want and defer the rest to the 
existing functions.

Also, I think get_named_text_section is a better name, in line with the
related functions.

Also, I wonder about the naming of these sections.  Why would we prefer
".text.foo_unlikely" rather than ".text.unlikely.foo"?  It seems to me that 
have the possibility of conflict between a function foo (which is unlikely),
and a function foo_unlikely (which happens to not be unlikely and so not
receive a suffix).

Also, it appears we're doing some amount of section switching even for
functions that have had their DECL_SECTION_NAME set by the user.  We don't
seem to be distinguishing between user settings and settings that came
from resolve_unique_section.  It seems to me that we should *never* change
the section when the user has specified one.

I realize that these last two are pre-existing conditions, but previously
would only be encountered with flag_reorder_blocks_and_partition.  You're
now planning changes that would take effect by default, which seems likely
to break kernels.

Which leads me to wonder if a new hash *is* called for, but would simply
be a map from decl to hot+cold section objects.  Perhaps

static section *
function_section_1 (tree decl, bool force_cold)
{
  slot = decl in hash;
  if (slot)
    {
      sec = is_cold ? slot->cold : slot->hot;
      if (sec)
	return sec;
    }
  else
    slot = new hash entry;

  if (targetm.have_named_sections)
    {
      if (DECL_SECTION_NAME (decl))
        {
          // The user has setup a section for the function.  Honor it.
          sec = get_named_section (decl, NULL, 0);
        }
      else if (DECL_ONE_ONLY (decl))
        {
          // The function must be placed in a comdat section.  Lest we
          // fail to merge duplicates, the section name is part of the
          // ABI of the function.  Don't do hot/cold splitting or other
          // sorts of renames.
	  // ??? Note that this still sets DECL_SECTION_NAME; ideally
	  // we'd revise the interface to return a section, but it's
	  // not critical.
          targetm.asm_out.unique_section (decl, false);
          sec = get_named_section (decl, NULL, 0);
	}
    }
  if (!sec)
    {
      // The hook handles all of the details of hot/cold/startup/exit.
      // It must *not* set DECL_SECTION_NAME, so that when we return
      // for the !is_cold section we arrive here and don't get confused
      // by the cases above.
      sec = targetm.asm_out.function_section (...);
    }

  if (is_cold)
    slot->cold = sec;
  else
    slot->hot = sec;

  return sec;
}



r~