Patchwork [PR,57084] Create real cgraph node during late intraprocedural devirtualization

login
register
mail settings
Submitter Martin Jambor
Date May 7, 2013, 5:55 p.m.
Message ID <20130507175546.GB3568@virgil.suse>
Download mbox | patch
Permalink /patch/242431/
State New
Headers show

Comments

Martin Jambor - May 7, 2013, 5:55 p.m.
Hi,

the problem in PR 57084 is that late PRE devirtualization creates a
direct call to a decl fro which we only have an inlined call graph
mode in the given partition.  I tried to find a most universal place
where to fix it because this problem is not special to type-based
devirtualization and in theory can be caused by any call to a decl
that is grabbed from a constructor.  I think the best place is the
following one-liner, because all such decls should go through
canonicalize_constructor_val.

Bootstrapped and tested on x86_64-linux, fixes the testcase (at -m32)
and I have happened to also LTO build Mozilla Firefox with it.  OK for
trunk?

Thanks,

Martin


2013-05-06  Martin Jambor  <mjambor@suse.cz>

	PR lto/57084
	* gimple-fold.c (canonicalize_constructor_val): Call
	cgraph_get_create_real_symbol_node instead of cgraph_get_create_node.
Jan Hubicka - May 7, 2013, 9:43 p.m.
> Hi,
> 
> the problem in PR 57084 is that late PRE devirtualization creates a
> direct call to a decl fro which we only have an inlined call graph
> mode in the given partition.  I tried to find a most universal place
> where to fix it because this problem is not special to type-based
> devirtualization and in theory can be caused by any call to a decl
> that is grabbed from a constructor.  I think the best place is the
> following one-liner, because all such decls should go through
> canonicalize_constructor_val.
> 
> Bootstrapped and tested on x86_64-linux, fixes the testcase (at -m32)
> and I have happened to also LTO build Mozilla Firefox with it.  OK for
> trunk?

Is this change needed for 4.8, too? It would be OK there.

For 4.9 I would preffer replacing cgraph_get_create by
cgraph_get_create_real_symbol_node.  I believe that all users of
cgraph_get_create are not interested in inline nodes. I just did not want to
make such intrusive change for 4.8...

Honza
Martin Jambor - May 9, 2013, 1:52 p.m.
On Tue, May 07, 2013 at 11:43:33PM +0200, Jan Hubicka wrote:
> > Hi,
> > 
> > the problem in PR 57084 is that late PRE devirtualization creates a
> > direct call to a decl fro which we only have an inlined call graph
> > mode in the given partition.  I tried to find a most universal place
> > where to fix it because this problem is not special to type-based
> > devirtualization and in theory can be caused by any call to a decl
> > that is grabbed from a constructor.  I think the best place is the
> > following one-liner, because all such decls should go through
> > canonicalize_constructor_val.
> > 
> > Bootstrapped and tested on x86_64-linux, fixes the testcase (at -m32)
> > and I have happened to also LTO build Mozilla Firefox with it.  OK for
> > trunk?
> 
> Is this change needed for 4.8, too? It would be OK there.

Although the testcase shows the problem relies on code that is new in
4.9, I think that yes, even the current devirtualization by looking
into constructors can hit the bug.  So I am going to commit it to 4.8
after testing on that branch.

> 
> For 4.9 I would preffer replacing cgraph_get_create by
> cgraph_get_create_real_symbol_node.  I believe that all users of
> cgraph_get_create are not interested in inline nodes. I just did not want to
> make such intrusive change for 4.8...

I've looked at all calls to cgraph_get_create_node and pretty much all
of them either really want cgraph_get_create_real_symbol_node or at
least will not be harmed by doing that.  The only two calls I was not
so sure about were the one in lto-cgraph.c and in particular the one
in lto-streamer-in.c.  I'm afraid they might create an extra cgraph
node in LTRANS when we would actually want the inlined one.  What do
you think?

I'll give it a go anyway.  Thanks,

Martin
Martin Jambor - May 9, 2013, 2:12 p.m.
On Thu, May 09, 2013 at 03:52:20PM +0200, Martin Jambor wrote:
> On Tue, May 07, 2013 at 11:43:33PM +0200, Jan Hubicka wrote:
> > > Hi,
> > > 
> > > the problem in PR 57084 is that late PRE devirtualization creates a
> > > direct call to a decl fro which we only have an inlined call graph
> > > mode in the given partition.  I tried to find a most universal place
> > > where to fix it because this problem is not special to type-based
> > > devirtualization and in theory can be caused by any call to a decl
> > > that is grabbed from a constructor.  I think the best place is the
> > > following one-liner, because all such decls should go through
> > > canonicalize_constructor_val.
> > > 
> > > Bootstrapped and tested on x86_64-linux, fixes the testcase (at -m32)
> > > and I have happened to also LTO build Mozilla Firefox with it.  OK for
> > > trunk?
> > 
> > Is this change needed for 4.8, too? It would be OK there.
> 
> Although the testcase shows the problem relies on code that is new in
> 4.9, I think that yes, even the current devirtualization by looking
> into constructors can hit the bug.  So I am going to commit it to 4.8
> after testing on that branch.

Except that there is no such thing as
cgraph_get_create_real_symbol_node in 4.8.  I suppose it's not worth
backporting it without a bug triggering there.  However, this makes me
think whether we should not commit the original one-line patch to
trunk first, even though we will zap
cgraph_get_create_real_symbol_node later, so that the patch can be
found and backported more easily if the need to backport it ever
arises...

Martin


> 
> > 
> > For 4.9 I would preffer replacing cgraph_get_create by
> > cgraph_get_create_real_symbol_node.  I believe that all users of
> > cgraph_get_create are not interested in inline nodes. I just did not want to
> > make such intrusive change for 4.8...
> 
> I've looked at all calls to cgraph_get_create_node and pretty much all
> of them either really want cgraph_get_create_real_symbol_node or at
> least will not be harmed by doing that.  The only two calls I was not
> so sure about were the one in lto-cgraph.c and in particular the one
> in lto-streamer-in.c.  I'm afraid they might create an extra cgraph
> node in LTRANS when we would actually want the inlined one.  What do
> you think?
> 
> I'll give it a go anyway.  Thanks,
> 
> Martin
>
Jan Hubicka - May 10, 2013, 11:49 a.m.
> On Thu, May 09, 2013 at 03:52:20PM +0200, Martin Jambor wrote:
> > On Tue, May 07, 2013 at 11:43:33PM +0200, Jan Hubicka wrote:
> > > > Hi,
> > > > 
> > > > the problem in PR 57084 is that late PRE devirtualization creates a
> > > > direct call to a decl fro which we only have an inlined call graph
> > > > mode in the given partition.  I tried to find a most universal place
> > > > where to fix it because this problem is not special to type-based
> > > > devirtualization and in theory can be caused by any call to a decl
> > > > that is grabbed from a constructor.  I think the best place is the
> > > > following one-liner, because all such decls should go through
> > > > canonicalize_constructor_val.
> > > > 
> > > > Bootstrapped and tested on x86_64-linux, fixes the testcase (at -m32)
> > > > and I have happened to also LTO build Mozilla Firefox with it.  OK for
> > > > trunk?
> > > 
> > > Is this change needed for 4.8, too? It would be OK there.
> > 
> > Although the testcase shows the problem relies on code that is new in
> > 4.9, I think that yes, even the current devirtualization by looking
> > into constructors can hit the bug.  So I am going to commit it to 4.8
> > after testing on that branch.
> 
> Except that there is no such thing as
> cgraph_get_create_real_symbol_node in 4.8.  I suppose it's not worth
> backporting it without a bug triggering there.  However, this makes me

Yep, it was same situation with the PR fix I introduced cgraph_get_create_real_symbol_node for.
In theory the bug exists for 4.8, too, but I do not have testcase.

Lets go with bugfixes first and then I can do the renaming.

Honza

Patch

Index: src/gcc/gimple-fold.c
===================================================================
--- src.orig/gcc/gimple-fold.c
+++ src/gcc/gimple-fold.c
@@ -178,7 +178,7 @@  canonicalize_constructor_val (tree cval,
 	  /* Make sure we create a cgraph node for functions we'll reference.
 	     They can be non-existent if the reference comes from an entry
 	     of an external vtable for example.  */
-	  cgraph_get_create_node (base);
+	  cgraph_get_create_real_symbol_node (base);
 	}
       /* Fixup types in global initializers.  */
       if (TREE_TYPE (TREE_TYPE (cval)) != TREE_TYPE (TREE_OPERAND (cval, 0)))