Re: Restore Tru64 UNIX bootstrap (PR middle-end/46671)

Re: Restore Tru64 UNIX bootstrap (PR middle-end/46671)

[Dominique Dhumieres]

Honza

> 	* varasm.c (default_function_section): Return NULL when
> 	flag_reorder_functions is disabled.
> 	* darwin.c (darwin_function_section): Likewise.
> 
> Index: varasm.c
> ===================================================================
> --- varasm.c	(revision 167472)
> +++ varasm.c	(working copy)
> @@ -533,6 +533,8 @@ section *
>  default_function_section (tree decl, enum node_frequency freq,
> 			  bool startup, bool exit)
>  {
> +  if (!flag_reorder_functions)
> +    return NULL;
>    /* Startup code should go to startup subsection unless it is
>       unlikely executed (this happens especially with function splitting
>       where we can split away unnecesary parts of static constructors.  */
> Index: config/darwin.c
> ===================================================================
> --- config/darwin.c	(revision 167472)
> +++ config/darwin.c	(working copy)
> @@ -2970,6 +2970,8 @@ section *
>  darwin_function_section (tree decl, enum node_frequency freq,
> 			  bool startup, bool exit)
>  {
> +  if (!flag_reorder_functions)
> +    return NULL;
>    /* Startup code should go to startup subsection unless it is
>       unlikely executed (this happens especially with function splitting
>       where we can split away unnecesary parts of static constructors.  */

caused http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46916 .

Dominique

Re: Restore Tru64 UNIX bootstrap (PR middle-end/46671)

[Jan Hubicka]

> Honza
> 
> > 	* varasm.c (default_function_section): Return NULL when
> > 	flag_reorder_functions is disabled.
> > 	* darwin.c (darwin_function_section): Likewise.
> > 
> > Index: varasm.c
> > ===================================================================
> > --- varasm.c	(revision 167472)
> > +++ varasm.c	(working copy)
> > @@ -533,6 +533,8 @@ section *
> >  default_function_section (tree decl, enum node_frequency freq,
> > 			  bool startup, bool exit)
> >  {
> > +  if (!flag_reorder_functions)
> > +    return NULL;
> >    /* Startup code should go to startup subsection unless it is
> >       unlikely executed (this happens especially with function splitting
> >       where we can split away unnecesary parts of static constructors.  */
> > Index: config/darwin.c
> > ===================================================================
> > --- config/darwin.c	(revision 167472)
> > +++ config/darwin.c	(working copy)
> > @@ -2970,6 +2970,8 @@ section *
> >  darwin_function_section (tree decl, enum node_frequency freq,
> > 			  bool startup, bool exit)
> >  {
> > +  if (!flag_reorder_functions)
> > +    return NULL;
> >    /* Startup code should go to startup subsection unless it is
> >       unlikely executed (this happens especially with function splitting
> >       where we can split away unnecesary parts of static constructors.  */
> 
> caused http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46916 .
Dominique, the change in question disables function reordering with -fno-function-reordering.
It is very weird that it causes problem like this.  I am flying home tonight so will look into
it tomorrow.

At first glance ICE in cgraph hashtable looks like memory corruption from
pobably some previously latent problem.  Is there something like valgrind or
other memory corruption checking software for darwin you can give a try?

Honza
> 
> Dominique

Re: Restore Tru64 UNIX bootstrap (PR middle-end/46671)

[Mike Stump]

On Dec 13, 2010, at 9:26 AM, Jan Hubicka wrote:
> Dominique, the change in question disables function reordering with -fno-function-reordering.
> It is very weird that it causes problem like this.  I am flying home tonight so will look into
> it tomorrow.

Before you spend too much time on this...  infinite recursion, see the PR.  Good news, there is also a proposed patch that has been Oked (once someone regression tests it).

Re: Restore Tru64 UNIX bootstrap (PR middle-end/46671)

[Jan Hubicka]

> 
> In my opinion, Rainer's change should also be committed as obvious.

I agree, I was just concerned bz this thread about non-elf targets.  So I will merge
both patches, re-test and commit.
Thanks!
Honza
> 
> Dave
> -- 
> J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
> National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: Restore Tru64 UNIX bootstrap (PR middle-end/46671)

[John David Anglin]

> > Ah, that explains why I started seeing the sections associated with
> > section reordering in default compilations without options.
> 
> -freorder-functions is now declared as { OPT_LEVELS_2_PLUS, OPT_freorder_functions, NULL, 1 },
> I do not see why this should not be default for -O1 too - it is cheap
> compilation time wise.  I am attaching the fix for flag_reorder_functions I
> intend to commit tonight as obvious if there are no complains

I was wrong about default options.  If I compile the following code
at -O1 and above on hppa-linux,

  int main () { return 0; }

main ends up in .text.startup.  In 4.5, it ends up in .text at all
levels that I tried.  With your change below, main will only end up
in .text.startup when flag_reorder_functions is true.  I believe
that is an improvement as it allows backends to override the behavior.

> 	* varasm.c (default_function_section): Return NULL when
> 	flag_reorder_functions is disabled.
> 	* darwin.c (darwin_function_section): Likewise.
> 
> Index: varasm.c
> ===================================================================
> --- varasm.c	(revision 167472)
> +++ varasm.c	(working copy)
> @@ -533,6 +533,8 @@ section *
>  default_function_section (tree decl, enum node_frequency freq,
>  			  bool startup, bool exit)
>  {
> +  if (!flag_reorder_functions)
> +    return NULL;
>    /* Startup code should go to startup subsection unless it is
>       unlikely executed (this happens especially with function splitting
>       where we can split away unnecesary parts of static constructors.  */
> Index: config/darwin.c
> ===================================================================
> --- config/darwin.c	(revision 167472)
> +++ config/darwin.c	(working copy)
> @@ -2970,6 +2970,8 @@ section *
>  darwin_function_section (tree decl, enum node_frequency freq,
>  			  bool startup, bool exit)
>  {
> +  if (!flag_reorder_functions)
> +    return NULL;
>    /* Startup code should go to startup subsection unless it is
>       unlikely executed (this happens especially with function splitting
>       where we can split away unnecesary parts of static constructors.  */

In my opinion, Rainer's change should also be committed as obvious.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: Restore Tru64 UNIX bootstrap (PR middle-end/46671)

[Jan Hubicka]

> > I lost the flag_reorder_functions check in my patch, will add it back into
> > default_function_section and dwarf2_function_section code.
> 
> Ah, that explains why I started seeing the sections associated with
> section reordering in default compilations without options.

-freorder-functions is now declared as { OPT_LEVELS_2_PLUS, OPT_freorder_functions, NULL, 1 },
I do not see why this should not be default for -O1 too - it is cheap
compilation time wise.  I am attaching the fix for flag_reorder_functions I
intend to commit tonight as obvious if there are no complains
> 
> We previously (4.5 branch) had the following code in choose_function_section:
> 
>   if (DECL_SECTION_NAME (current_function_decl)
>       || !targetm.have_named_sections
>       /* Theoretically we can split the gnu.linkonce text section too,
>       but this requires more work as the frequency needs to match
>       for all generated objects so we need to merge the frequency
>       of all instances.  For now just never set frequency for these.  */
>       || DECL_ONE_ONLY (current_function_decl))
>    return;

I see, I missed the targetm.have_named_sections.  I guess the proposed patch still
makes sense in this context.
> 
>    /* If we are doing the partitioning optimization, let the optimization
>       choose the correct section into which to put things.  */
> 
>    if (flag_reorder_blocks_and_partition)
>      return;
> 
> It appears functions with a named section are correctly handled by the
> new code by name concatenation.  Don't know about DECL_ONE_ONLY and
> flag_reorder_blocks_and_partition (latter is disabled on PA).

The concatenation code comes from flag_reorder_blocks_and_partition code.
there is code checking if -freorder-blocks-and-partition is supposed and
it also uses the targetm.have_named_sections.  So I suppose on named section
targets we did the renaming.

I am however not too opposed to making this default for ELF systems only.
We already handle this specially on darwin, that leaves us with cygwin
as a target where we probably really care?

Honza

	* varasm.c (default_function_section): Return NULL when
	flag_reorder_functions is disabled.
	* darwin.c (darwin_function_section): Likewise.

Index: varasm.c
===================================================================
--- varasm.c	(revision 167472)
+++ varasm.c	(working copy)
@@ -533,6 +533,8 @@ section *
 default_function_section (tree decl, enum node_frequency freq,
 			  bool startup, bool exit)
 {
+  if (!flag_reorder_functions)
+    return NULL;
   /* Startup code should go to startup subsection unless it is
      unlikely executed (this happens especially with function splitting
      where we can split away unnecesary parts of static constructors.  */
Index: config/darwin.c
===================================================================
--- config/darwin.c	(revision 167472)
+++ config/darwin.c	(working copy)
@@ -2970,6 +2970,8 @@ section *
 darwin_function_section (tree decl, enum node_frequency freq,
 			  bool startup, bool exit)
 {
+  if (!flag_reorder_functions)
+    return NULL;
   /* Startup code should go to startup subsection unless it is
      unlikely executed (this happens especially with function splitting
      where we can split away unnecesary parts of static constructors.  */

Re: Restore Tru64 UNIX bootstrap (PR middle-end/46671)

[John David Anglin]

> I lost the flag_reorder_functions check in my patch, will add it back into
> default_function_section and dwarf2_function_section code.

Ah, that explains why I started seeing the sections associated with
section reordering in default compilations without options.

We previously (4.5 branch) had the following code in choose_function_section:

  if (DECL_SECTION_NAME (current_function_decl)
      || !targetm.have_named_sections
      /* Theoretically we can split the gnu.linkonce text section too,
      but this requires more work as the frequency needs to match
      for all generated objects so we need to merge the frequency
      of all instances.  For now just never set frequency for these.  */
      || DECL_ONE_ONLY (current_function_decl))
   return;

   /* If we are doing the partitioning optimization, let the optimization
      choose the correct section into which to put things.  */

   if (flag_reorder_blocks_and_partition)
     return;

It appears functions with a named section are correctly handled by the
new code by name concatenation.  Don't know about DECL_ONE_ONLY and
flag_reorder_blocks_and_partition (latter is disabled on PA).

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: Restore Tru64 UNIX bootstrap (PR middle-end/46671)

[Jan Hubicka]

> > > 	* varasm.c (default_function_section): Return NULL unless
> > > 	targetm.have_named_sections.
> > > --- a/gcc/varasm.c	Fri Dec 10 17:50:52 2010 +0100
> > > +++ b/gcc/varasm.c	Fri Dec 10 17:52:57 2010 +0100
> > > @@ -533,6 +533,9 @@
> > >  default_function_section (tree decl, enum node_frequency freq,
> > >  			  bool startup, bool exit)
> > >  {
> > > +  if (!targetm.have_named_sections)
> > > +    return NULL;
> > 
> > Yes, this seems sane default for me.
> 
> I think this doesn't go far enough in restoring previous behaviour.
> There may be targets with named sections that aren't ELF.
> 
> The current implementation of default_function_section is really
> ELF/GNU ld specific.  I think it would be better if it were changed
> to elf_default_function_section selected in elfos.h.  The default for

OK, I see that currently only GNU linker does the ordering, but the previous
code was enabled by default on non-elf targets too?
choose_function was called with flag_reorder_function and set DECL_NAMED_SECTION
that in my understanding affects all targets, elf or non-elf.

I lost the flag_reorder_functions check in my patch, will add it back into
default_function_section and dwarf2_function_section code.

Honza

> TARGET_ASM_FUNCTION_SECTION could be NULL so the hook isn't called
> at all for the default case.  The code would then fall through to
> hot_function_section.  This probably could be further simplified.
> 
> I am probably stuck in defining TARGET_ASM_FUNCTION_SECTION on PA
> because of a problem in handling nested functions when they are in
> different sections...
> 
> Dave
> -- 
> J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
> National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: Restore Tru64 UNIX bootstrap (PR middle-end/46671)

[John David Anglin]

> > 	* varasm.c (default_function_section): Return NULL unless
> > 	targetm.have_named_sections.
> > --- a/gcc/varasm.c	Fri Dec 10 17:50:52 2010 +0100
> > +++ b/gcc/varasm.c	Fri Dec 10 17:52:57 2010 +0100
> > @@ -533,6 +533,9 @@
> >  default_function_section (tree decl, enum node_frequency freq,
> >  			  bool startup, bool exit)
> >  {
> > +  if (!targetm.have_named_sections)
> > +    return NULL;
> 
> Yes, this seems sane default for me.

I think this doesn't go far enough in restoring previous behaviour.
There may be targets with named sections that aren't ELF.

The current implementation of default_function_section is really
ELF/GNU ld specific.  I think it would be better if it were changed
to elf_default_function_section selected in elfos.h.  The default for
TARGET_ASM_FUNCTION_SECTION could be NULL so the hook isn't called
at all for the default case.  The code would then fall through to
hot_function_section.  This probably could be further simplified.

I am probably stuck in defining TARGET_ASM_FUNCTION_SECTION on PA
because of a problem in handling nested functions when they are in
different sections...

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: Restore Tru64 UNIX bootstrap (PR middle-end/46671)

[Jan Hubicka]

> Jan's patch
> 
> 	Re: Group static constructors and destructors in specific subsections, take 2
>         http://gcc.gnu.org/ml/gcc-patches/2010-11/msg00983.html
> 
> broke bootstrap on every target without named sections since varasm.c
> (default_function_section) assumes they exist.  While two port
> maintainers so far have worked around the breakage in their ports
> (pdp11, hppa) since Jan completely failed to address (or even
> acknowledge) the breakage he's caused, I think the fix belongs into
> default_function_section to make that function an appropriate default.

Sorry, I was offline for last week and didn't noticed the problem before leaving.
Yes, it is indeed problem introduce by my patch.

> 	* varasm.c (default_function_section): Return NULL unless
> 	targetm.have_named_sections.
> --- a/gcc/varasm.c	Fri Dec 10 17:50:52 2010 +0100
> +++ b/gcc/varasm.c	Fri Dec 10 17:52:57 2010 +0100
> @@ -533,6 +533,9 @@
>  default_function_section (tree decl, enum node_frequency freq,
>  			  bool startup, bool exit)
>  {
> +  if (!targetm.have_named_sections)
> +    return NULL;

Yes, this seems sane default for me.

Thanks!
Honza
> +
>    /* Startup code should go to startup subsection unless it is
>       unlikely executed (this happens especially with function splitting
>       where we can split away unnecesary parts of static constructors.  */
> 
> 
> -- 
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Restore Tru64 UNIX bootstrap (PR middle-end/46671)

[Jakub Jelinek]

On Fri, Dec 10, 2010 at 06:29:12PM +0100, Rainer Orth wrote:
> 2010-12-09  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
> 
> 	PR middle-end/46671
> 	* varasm.c (default_function_section): Return NULL unless
> 	targetm.have_named_sections.
> 

This:
> 	* regrename.c (check_new_reg_p): Mark reg with ATTRIBUTE_UNUSED.

hunk is PR46844, fixed for 2 days already...
> 
> diff -r 0168da119e76 gcc/regrename.c
> --- a/gcc/regrename.c	Fri Dec 10 17:50:52 2010 +0100
> +++ b/gcc/regrename.c	Fri Dec 10 17:52:57 2010 +0100
> @@ -309,8 +309,8 @@
>     registers.  */
>  
>  static bool
> -check_new_reg_p (int reg, int new_reg,  struct du_head *this_head,
> -		 HARD_REG_SET this_unavailable)
> +check_new_reg_p (int reg ATTRIBUTE_UNUSED, int new_reg,
> +		 struct du_head *this_head, HARD_REG_SET this_unavailable)
>  {
>    enum machine_mode mode = GET_MODE (*this_head->first->loc);
>    int nregs = hard_regno_nregs[new_reg][mode];

	Jakub

Restore Tru64 UNIX bootstrap (PR middle-end/46671)

[Rainer Orth]

Jan's patch

	Re: Group static constructors and destructors in specific subsections, take 2
        http://gcc.gnu.org/ml/gcc-patches/2010-11/msg00983.html

broke bootstrap on every target without named sections since varasm.c
(default_function_section) assumes they exist.  While two port
maintainers so far have worked around the breakage in their ports
(pdp11, hppa) since Jan completely failed to address (or even
acknowledge) the breakage he's caused, I think the fix belongs into
default_function_section to make that function an appropriate default.

The patch below does this, and also includes another obvious fix to
allow the bootstrap to continue.  Bootstrap has finished with the
patch.  While testing is still underway, there were no unusual testsuite
failures so far.

Ok for mainline?

	Rainer


2010-12-09  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	PR middle-end/46671
	* varasm.c (default_function_section): Return NULL unless
	targetm.have_named_sections.

	* regrename.c (check_new_reg_p): Mark reg with ATTRIBUTE_UNUSED.

diff -r 0168da119e76 gcc/regrename.c
--- a/gcc/regrename.c	Fri Dec 10 17:50:52 2010 +0100
+++ b/gcc/regrename.c	Fri Dec 10 17:52:57 2010 +0100
@@ -309,8 +309,8 @@
    registers.  */
 
 static bool
-check_new_reg_p (int reg, int new_reg,  struct du_head *this_head,
-		 HARD_REG_SET this_unavailable)
+check_new_reg_p (int reg ATTRIBUTE_UNUSED, int new_reg,
+		 struct du_head *this_head, HARD_REG_SET this_unavailable)
 {
   enum machine_mode mode = GET_MODE (*this_head->first->loc);
   int nregs = hard_regno_nregs[new_reg][mode];
diff -r 0168da119e76 gcc/varasm.c
--- a/gcc/varasm.c	Fri Dec 10 17:50:52 2010 +0100
+++ b/gcc/varasm.c	Fri Dec 10 17:52:57 2010 +0100
@@ -533,6 +533,9 @@
 default_function_section (tree decl, enum node_frequency freq,
 			  bool startup, bool exit)
 {
+  if (!targetm.have_named_sections)
+    return NULL;
+
   /* Startup code should go to startup subsection unless it is
      unlikely executed (this happens especially with function splitting
      where we can split away unnecesary parts of static constructors.  */


-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University