diff mbox

Make dominated_by_p and get_immediate_dominator inline

Message ID 20100623101545.GA9710@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka June 23, 2010, 10:15 a.m. UTC 
> On Tue, Jun 22, 2010 at 8:36 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> > Hi,
> > this is change I forgot in my tree for a while.  get_immediate_dominator and
> > dominated_by_p are one of most frequently called functions and they are good
> > candidates for inlining. Dominated_by_p has fallback path calling et_forest
> > code.  Most of passes don't use dynamic dominance info and thus we never get
> > there.  I was thinking about making specific version for non-dynamic dominators,
> > but since the most common calls from dominated_by_p comes from cfg.c
> > (dominance frontiers computation) that is sort of generic, it would
> > require more bookkeping of when info is dynamic and when not.
> > Hopefully the cold path is not going to cause too much of trouble.
> >
> > The patch needs to include et-forest.h in basic-block.h, but I do not see
> > much way around.
> 
> Do not make this inline?
> 
> I think making all these small functions inline makes things only more
> entangled. Now you include et-forest.h in every place that includes
> basic-block.h. I see no point in continuing my little project of
> removing #include's if you plan to add more of these.
> 
> I would much rather *remove* inline functions and let WHOPR do its job!

I am defnitly trying to push things towards making WHOPR the official way
to build GCC binary.  Hopefully we will get there for next release.

Even with WHOPR we won't inline dominated_by_p, at least as long as we
build with -O2 because inlining it increases cc1 binary size (by 14kb).
So at least we would have to declare it inline in dominance.c

Averaging 5 runs of each I got 9m4s withtout the patch and 9m2s with the
patch. dominated_by_p accounts to about 0.4% of runtime according to oprofile.
It seems that we do too many queries using et_splay that acounts 0.56% of
compilation time.  I guess it is because we do not recompute and keep
dominance tree in NO_FAST_QUERY mode for a while.  Will check.

Well, if we agree that we do not care about this relatively little amount
of perfomrance for non-WHOPR build, I guess following patch would be
less intrusive ;)

Bootstrapping/regtesting x86_64-linux, OK?

Honza

	* dominance.c (get_immediate_dominator, set_immediate_dominator,
	dom_convert_dir_to_idx): Make inline; use checking assert.

Comments

Joseph Myers June 23, 2010, 11:30 a.m. UTC | #1 
On Wed, 23 Jun 2010, Jan Hubicka wrote:

> I am defnitly trying to push things towards making WHOPR the official way
> to build GCC binary.  Hopefully we will get there for next release.

Is the expectation that WHOPR builds of GCC will require GCC 4.6 (when 
building a cross compiler, say)?

The present GCC build system puts various objects in .a files.  Does this 
mean that WHOPR build of GCC requires LTO support for .a files, and so 
needs the linker plugin at present and does not work with GNU ld (for 
non-ELF hosts not supported by gold, for example) unless new features are 
added to GNU ld to support it (I hope such features are added to GNU ld)?
Richard Biener June 23, 2010, 12:24 p.m. UTC | #2 
On Wed, Jun 23, 2010 at 1:30 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Wed, 23 Jun 2010, Jan Hubicka wrote:
>
>> I am defnitly trying to push things towards making WHOPR the official way
>> to build GCC binary.  Hopefully we will get there for next release.
>
> Is the expectation that WHOPR builds of GCC will require GCC 4.6 (when
> building a cross compiler, say)?

If we don't want to bootstrap a compiler for the host first then yes.

> The present GCC build system puts various objects in .a files.  Does this
> mean that WHOPR build of GCC requires LTO support for .a files, and so
> needs the linker plugin at present and does not work with GNU ld (for
> non-ELF hosts not supported by gold, for example) unless new features are
> added to GNU ld to support it (I hope such features are added to GNU ld)?

ISTR somebody was working on teaching GNU ld to emit a resolution
file.  See the patch from Dave Korn in PR41376.  I don't know if
that was proposed to the binutils list yet.

Richard.

> --
> Joseph S. Myers
> joseph@codesourcery.com
>
Jan Hubicka June 23, 2010, 12:41 p.m. UTC | #3 
> On Wed, Jun 23, 2010 at 1:30 PM, Joseph S. Myers
> <joseph@codesourcery.com> wrote:
> > On Wed, 23 Jun 2010, Jan Hubicka wrote:
> >
> >> I am defnitly trying to push things towards making WHOPR the official way
> >> to build GCC binary.  Hopefully we will get there for next release.
> >
> > Is the expectation that WHOPR builds of GCC will require GCC 4.6 (when
> > building a cross compiler, say)?
> 
> If we don't want to bootstrap a compiler for the host first then yes.
> 
> > The present GCC build system puts various objects in .a files.  Does this
> > mean that WHOPR build of GCC requires LTO support for .a files, and so
> > needs the linker plugin at present and does not work with GNU ld (for
> > non-ELF hosts not supported by gold, for example) unless new features are
> > added to GNU ld to support it (I hope such features are added to GNU ld)?
> 
> ISTR somebody was working on teaching GNU ld to emit a resolution
> file.  See the patch from Dave Korn in PR41376.  I don't know if
> that was proposed to the binutils list yet.

Option would be to modify our build system to not use libbackend.a that might
be feasible solution for 4.6 if LD is not ready for plugins.  With GOLD we link
into GCC libiberty, libcpp and other stuff, but as far as I know linking
backend + fronends with -fwhole-file should just work. (i.e. libiberty and libcpp
are quite well behaved libraries)

Honza
> 
> Richard.
> 
> > --
> > Joseph S. Myers
> > joseph@codesourcery.com
> >
diff mbox

Patch

Index: dominance.c
===================================================================
--- dominance.c	(revision 161199)
+++ dominance.c	(working copy)
@@ -188,10 +188,10 @@  init_dom_info (struct dom_info *di, enum
    to be modified, obviously, if additional values are added to
    cdi_direction.  */
 
-static unsigned int
+static inline unsigned int
 dom_convert_dir_to_idx (enum cdi_direction dir)
 {
-  gcc_assert (dir == CDI_DOMINATORS || dir == CDI_POST_DOMINATORS);
+  gcc_checking_assert (dir == CDI_DOMINATORS || dir == CDI_POST_DOMINATORS);
   return dir - 1;
 }
 
@@ -696,13 +696,13 @@  free_dominance_info (enum cdi_direction
 }
 
 /* Return the immediate dominator of basic block BB.  */
-basic_block
+inline basic_block
 get_immediate_dominator (enum cdi_direction dir, basic_block bb)
 {
   unsigned int dir_index = dom_convert_dir_to_idx (dir);
   struct et_node *node = bb->dom[dir_index];
 
-  gcc_assert (dom_computed[dir_index]);
+  gcc_checking_assert (dom_computed[dir_index]);
 
   if (!node->father)
     return NULL;
@@ -712,14 +712,14 @@  get_immediate_dominator (enum cdi_direct
 
 /* Set the immediate dominator of the block possibly removing
    existing edge.  NULL can be used to remove any edge.  */
-void
+inline void
 set_immediate_dominator (enum cdi_direction dir, basic_block bb,
 			 basic_block dominated_by)
 {
   unsigned int dir_index = dom_convert_dir_to_idx (dir);
   struct et_node *node = bb->dom[dir_index];
 
-  gcc_assert (dom_computed[dir_index]);
+  gcc_checking_assert (dom_computed[dir_index]);
 
   if (node->father)
     {