| Message ID | 20130507175546.GB3568@virgil.suse |
|---|---|
| State | New |
| Headers | show |
> 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
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
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 >
> 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
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)))