Message ID | 20130116100127.GA21071@virgil.suse |
---|---|
State | New |
Headers | show |
On Wed, Jan 16, 2013 at 11:01 AM, Martin Jambor <mjambor@suse.cz> wrote: > Hi, > > PR 55264 is caused by cgraph machinery thinking it knows all calls to > a virtual method when that is actually not true. As discussed with > Honza, prior to inlining, we should not assume some virtual functions > (namely those that are neither DECL_COMDAT nor DECL_EXTERNAL) are not > reachable. Are they not reachable from the VTABLE which is referenced from all calls that eventually reach the virtual method? Thus, isn't the issue that the VTABLE is not correctly handled by ipa-references code? > DECL_EXTERNAL however still affects the cgraph_node->local.local flag > and so in order to avoid some LTO failures, I had to adjust IPA-CP to > consider such virtual functions non-local so that verification of > lattice propagation does not complain. I'm a bit puzzled by the value > of the flag in this situation but at least it does not seem to cause > any other problems. > > Below is a trunk patch to do all this. It does not apply to released > versions which also suffer from the same problem so I'll prepare a > special version(s) for them once this gets approved. Bootstrapped and > tested on x86_64-linux. OK for trunk? > > Thanks, > > Martin > > > 2013-01-15 Martin Jambor <mjambor@suse.cz> > > PR tree-optimizations/55264 > * ipa-inline-transform.c (can_remove_node_now_p_1): Never return true > for virtual methods. > * ipa.c (symtab_remove_unreachable_nodes): Never return true for > virtual methods before inlining is over. > * ipa-cp.c (initialize_node_lattices): Also consider all virtual > methods non-local. > > testsuite/ > * g++.dg/ipa/pr55264.C: New test. > > Index: src/gcc/ipa-inline-transform.c > =================================================================== > --- src.orig/gcc/ipa-inline-transform.c > +++ src/gcc/ipa-inline-transform.c > @@ -92,9 +92,7 @@ can_remove_node_now_p_1 (struct cgraph_n > those only after all devirtualizable virtual calls are processed. > Lacking may edges in callgraph we just preserve them post > inlining. */ > - && (!DECL_VIRTUAL_P (node->symbol.decl) > - || (!DECL_COMDAT (node->symbol.decl) > - && !DECL_EXTERNAL (node->symbol.decl))) > + && !DECL_VIRTUAL_P (node->symbol.decl) > /* During early inlining some unanalyzed cgraph nodes might be in the > callgraph and they might reffer the function in question. */ > && !cgraph_new_nodes); > Index: src/gcc/ipa.c > =================================================================== > --- src.orig/gcc/ipa.c > +++ src/gcc/ipa.c > @@ -241,8 +241,7 @@ symtab_remove_unreachable_nodes (bool be > && (!cgraph_can_remove_if_no_direct_calls_and_refs_p (node) > /* Keep around virtual functions for possible devirtualization. */ > || (before_inlining_p > - && DECL_VIRTUAL_P (node->symbol.decl) > - && (DECL_COMDAT (node->symbol.decl) || DECL_EXTERNAL (node->symbol.decl))))) > + && DECL_VIRTUAL_P (node->symbol.decl)))) > { > gcc_assert (!node->global.inlined_to); > pointer_set_insert (reachable, node); > Index: src/gcc/testsuite/g++.dg/ipa/pr55264.C > =================================================================== > --- /dev/null > +++ src/gcc/testsuite/g++.dg/ipa/pr55264.C > @@ -0,0 +1,17 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fno-early-inlining -fno-weak" } */ > + > +struct S > +{ > + S(); > + virtual inline void foo () > + { > + foo(); > + } > +}; > + > +void > +B () > +{ > + S().foo (); > +} > Index: src/gcc/ipa-cp.c > =================================================================== > --- src.orig/gcc/ipa-cp.c > +++ src/gcc/ipa-cp.c > @@ -699,7 +699,8 @@ initialize_node_lattices (struct cgraph_ > int i; > > gcc_checking_assert (cgraph_function_with_gimple_body_p (node)); > - if (!node->local.local) > + if (!node->local.local > + || DECL_VIRTUAL_P (node->symbol.decl)) > { > /* When cloning is allowed, we can assume that externally visible > functions are not called. We will compensate this by cloning
> On Wed, Jan 16, 2013 at 11:01 AM, Martin Jambor <mjambor@suse.cz> wrote: > > Hi, > > > > PR 55264 is caused by cgraph machinery thinking it knows all calls to > > a virtual method when that is actually not true. As discussed with > > Honza, prior to inlining, we should not assume some virtual functions > > (namely those that are neither DECL_COMDAT nor DECL_EXTERNAL) are not > > reachable. > > Are they not reachable from the VTABLE which is referenced from all > calls that eventually reach the virtual method? Thus, isn't the issue that > the VTABLE is not correctly handled by ipa-references code? No, the problem is that VTABLE can be keyed to other unit and not present in current unit at all and devirtualization is possible only via BINFOs. We already handle COMDAT/EXTERNALs like this. In longer run, I think we should build "may" edges for virtual calls that will render the corresponding methods reachable. > > > DECL_EXTERNAL however still affects the cgraph_node->local.local flag > > and so in order to avoid some LTO failures, I had to adjust IPA-CP to > > consider such virtual functions non-local so that verification of > > lattice propagation does not complain. I'm a bit puzzled by the value > > of the flag in this situation but at least it does not seem to cause > > any other problems. Hmm, perhaps we could simply set local flag to be false for external functions? The local flag is mostly used by codegen at a time the external functions are either inlined or removed, so it is never used in that context. Perhaps could you first change cgraph_non_local_node_p_1 and try to check some code if codegen differs significantly? It should not at all. ipa-cp is the sole user of this flag in IPA passes, so you should know what it does. > > > > Index: src/gcc/ipa-inline-transform.c > > =================================================================== > > --- src.orig/gcc/ipa-inline-transform.c > > +++ src/gcc/ipa-inline-transform.c > > @@ -92,9 +92,7 @@ can_remove_node_now_p_1 (struct cgraph_n > > those only after all devirtualizable virtual calls are processed. > > Lacking may edges in callgraph we just preserve them post > > inlining. */ > > - && (!DECL_VIRTUAL_P (node->symbol.decl) > > - || (!DECL_COMDAT (node->symbol.decl) > > - && !DECL_EXTERNAL (node->symbol.decl))) > > + && !DECL_VIRTUAL_P (node->symbol.decl) > > /* During early inlining some unanalyzed cgraph nodes might be in the > > callgraph and they might reffer the function in question. */ > > && !cgraph_new_nodes); > > Index: src/gcc/ipa.c > > =================================================================== > > --- src.orig/gcc/ipa.c > > +++ src/gcc/ipa.c > > @@ -241,8 +241,7 @@ symtab_remove_unreachable_nodes (bool be > > && (!cgraph_can_remove_if_no_direct_calls_and_refs_p (node) > > /* Keep around virtual functions for possible devirtualization. */ > > || (before_inlining_p > > - && DECL_VIRTUAL_P (node->symbol.decl) > > - && (DECL_COMDAT (node->symbol.decl) || DECL_EXTERNAL (node->symbol.decl))))) > > + && DECL_VIRTUAL_P (node->symbol.decl)))) > > { > > gcc_assert (!node->global.inlined_to); > > pointer_set_insert (reachable, node); > > Index: src/gcc/testsuite/g++.dg/ipa/pr55264.C > > =================================================================== > > --- /dev/null > > +++ src/gcc/testsuite/g++.dg/ipa/pr55264.C > > @@ -0,0 +1,17 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-O2 -fno-early-inlining -fno-weak" } */ > > + > > +struct S > > +{ > > + S(); > > + virtual inline void foo () > > + { > > + foo(); > > + } > > +}; > > + > > +void > > +B () > > +{ > > + S().foo (); > > +} These changes are OK. > > Index: src/gcc/ipa-cp.c > > =================================================================== > > --- src.orig/gcc/ipa-cp.c > > +++ src/gcc/ipa-cp.c > > @@ -699,7 +699,8 @@ initialize_node_lattices (struct cgraph_ > > int i; > > > > gcc_checking_assert (cgraph_function_with_gimple_body_p (node)); > > - if (!node->local.local) > > + if (!node->local.local > > + || DECL_VIRTUAL_P (node->symbol.decl)) > > { > > /* When cloning is allowed, we can assume that externally visible > > functions are not called. We will compensate this by cloning As mentioned above I would preffer the nonlocal_node_p change. If it passes testing and does not regress, consider the patch pre-approved. Honza
> Perhaps could you first change cgraph_non_local_node_p_1 and try to check some code > if codegen differs significantly? It should not at all. > ipa-cp is the sole user of this flag in IPA passes, so you should know what it does. Thinking deeper of ipa-cp and local virtuals, I think this is all slipperly. Local means that all calls to the functions are explicit and known. Obviously if function is virutal and new calls may appear by devirtualization, the local flag is bogus. I guess the external functions are the only that may be local and virtual because somewhere there must be a vtable reference, but to play safe, I would suggest marking all virtuals non-local. Honza
Index: src/gcc/ipa-inline-transform.c =================================================================== --- src.orig/gcc/ipa-inline-transform.c +++ src/gcc/ipa-inline-transform.c @@ -92,9 +92,7 @@ can_remove_node_now_p_1 (struct cgraph_n those only after all devirtualizable virtual calls are processed. Lacking may edges in callgraph we just preserve them post inlining. */ - && (!DECL_VIRTUAL_P (node->symbol.decl) - || (!DECL_COMDAT (node->symbol.decl) - && !DECL_EXTERNAL (node->symbol.decl))) + && !DECL_VIRTUAL_P (node->symbol.decl) /* During early inlining some unanalyzed cgraph nodes might be in the callgraph and they might reffer the function in question. */ && !cgraph_new_nodes); Index: src/gcc/ipa.c =================================================================== --- src.orig/gcc/ipa.c +++ src/gcc/ipa.c @@ -241,8 +241,7 @@ symtab_remove_unreachable_nodes (bool be && (!cgraph_can_remove_if_no_direct_calls_and_refs_p (node) /* Keep around virtual functions for possible devirtualization. */ || (before_inlining_p - && DECL_VIRTUAL_P (node->symbol.decl) - && (DECL_COMDAT (node->symbol.decl) || DECL_EXTERNAL (node->symbol.decl))))) + && DECL_VIRTUAL_P (node->symbol.decl)))) { gcc_assert (!node->global.inlined_to); pointer_set_insert (reachable, node); Index: src/gcc/testsuite/g++.dg/ipa/pr55264.C =================================================================== --- /dev/null +++ src/gcc/testsuite/g++.dg/ipa/pr55264.C @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fno-early-inlining -fno-weak" } */ + +struct S +{ + S(); + virtual inline void foo () + { + foo(); + } +}; + +void +B () +{ + S().foo (); +} Index: src/gcc/ipa-cp.c =================================================================== --- src.orig/gcc/ipa-cp.c +++ src/gcc/ipa-cp.c @@ -699,7 +699,8 @@ initialize_node_lattices (struct cgraph_ int i; gcc_checking_assert (cgraph_function_with_gimple_body_p (node)); - if (!node->local.local) + if (!node->local.local + || DECL_VIRTUAL_P (node->symbol.decl)) { /* When cloning is allowed, we can assume that externally visible functions are not called. We will compensate this by cloning