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

[Jan Hubicka <hubicka at ucw dot cz>]

> > -#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.
OK, will do.  Basically it should return NULL.
> 
> >  #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.

Hmm, yep, I was looking into that too, but it seems better to do this incrementally
rather than keep snowballing this.
> 
> 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.

Well, I wanted to avoid the string operations because old code did that
too.  We can switch quite few times when doing BB splitting, but I guess
it is non-critical whether we will always concatenate or whether we will
cache in hash.  I can get rid of it definitly.
> 
> Also, I think get_named_text_section is a better name, in line with the
> related functions.
OK.  I use subsection because...
> 
> 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).

Hmm, this was again copied from old code.  I
> 
> 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.

... while i was glancing over this I believed that it is supposed to work
in a way that when user specify named section "foo", we will create
foo._unlikely/foo._hot ... etc subsection so if there are multiple functions in
the same section they willbe properly grouped within that function.
> 
> 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.

We used to produce cold functions automatically since ipa-profile got in.
> 
> 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

I was trying to avoid too much assumptions that we break up into hot/cold
only since it makes sense to do more partitioning and there is no need
to spread this across the code.  But this would work with me, too.

Honza
> 
> 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~