| Message ID | 20101122220556.GB21794@virgil.arch.suse.de |
|---|---|
| State | New |
| Headers | show |
On Mon, 22 Nov 2010, Martin Jambor wrote: > Hi, > > this patch fixes a wide range of issues including PR 45934 which are > caused by the fact that the dynamic type of object changes during > construction and destruction and devirtualization has to take these > changes into account and has to avoid creating calls to descendants' > implementation of a virtual functions when a constructor or a > destructor of an ancestor is running. This is achieved by doing two > things: > > 1) We cannot devirtualize OBJ_TYPE_REFs according to the static type > of the object during folding. This code was dumbed down to what gcc > 4.5 does, which basically devirtualizes only when the O_T_R object is > a declaration - as opposed to an ancestor field within a declaration. > This is necessary to make g++.dg/opt/devirt1.C pass and is safe. Note > that we will want to fold O_T_Rs to their first parameter and we are > currently not that far from it. > > Nevertheless, it also makes other testcases fail, specifically > g++.dg/otr-fold-1.C, g++.dg/otr-fold-2.C, g++.dg/tree-ssa/pr43411.C > and g++.dg/tree-ssa/pr45605.C. These testcases depend on type-based > devirtualization and I will post a followup (RFC) patch that deals > with them at -O2. This probably should be split out into a separate patch. > 2) We can never devirtualize according to a type of a global object > because we do not know whether or not the current function has been > (perhaps indirectly) called from its constructor or destructor. This > also means that we cannot devirtualize according to types of constants > propagated by IPA-CP because those are always global object. > Therefore I removed code paths deriving types from constant IPA-CP > lattices and I check that declarations are not global when extracting > BINFOs from them. > > This also means that testcases g++.dg/ipa/ipcp-ivi-1.C and > ivinline-6.C test something which cannot be safely done and therefore > I propose to remove them. Likewise. > 3) Whenever IPA_JF_KNOWN_TYPE, no-op IPA_JF_PASS_THROUGH or > IPA_JF_ANCESTOR jump functions are constructed, we also walk VDEFS and > look for preceeding code that actually changes its dynamic type by > assigning to its virtual method table pointer. From the RHS we can > then identify the new type of the object and construct an appropriate > IPA_JF_KNOWN_TYPE jump function instead. > > This patch does not attempt to limit walking of VDEFs, I left that for > later. I am also not quite sure how to do that. During the body scan > that ipa-prop.c does to do modification analysis it could look for > assignments with a RHS that looks like a VMT address (address of an > array ref into a variable declaration which is DECL_VIRTUAL), and walk > VDEFS only if it finds one. The potentially inefficient VDEF walking > would then happen only in constructors, destructors and functions into > which they were inlined by early inlining (and we could punt for big > functions like that). I doubt you can conservatively identify stores to virtual method table pointers (well, w/o identifying every store as such). You also have to assume there is such store elsewhere if you do _not_ find such a store. But anyway - this part doesn't seem to be addressed in the current patch at all, right? > This patch also replaces all remaining uses of > gimple_get_relevant_ref_binfo with a combination of > get_base_ref_and_extent and get_binfo_at_offset because it is actually > convenient and it is easy to have the two binfo searching functions > diverge even though they should not. It probably makes sense to split this piece into a separate patch. > I assume there will be comments and suggestions. Nevertheless, the > patch does pass bootstrap and regression tests (except for the > testcases described above) on x86_64-linux and also make check-c++ on > i686. Mind that it has to be applied on top of my fix for PR 46053 > (http://gcc.gnu.org/ml/gcc-patches/2010-11/msg02291.html). Indeed. Comments in-line. > Thanks, > > Martin > > > > 2010-11-19 Martin Jambor <mjambor@suse.cz> > > PR tree-optimization/45934 > * gimple-fold.c (get_base_binfo_for_type): Removed. > (gimple_get_relevant_ref_binfo): Likewise. > (gimple_fold_obj_type_ref_call): Dumb down to 4.5 functionality, > removed parameter inplace, updated the caller. > * ipa-cp.c (ipcp_propagate_types): Do not derive types from constants. > (ipcp_discover_new_direct_edges): Do not do devirtualization based on > constants. > * gimple.h (gimple_get_relevant_ref_binfo): Remove declaration. > * tree.c (get_binfo_at_offset): Get type from non-artificial fields. > * ipa-prop.c (check_type_change): New function. > (detect_type_change_1): Likewise. > (detect_type_change): Likewise. > (compute_complex_assign_jump_func): Check dynamic type change. > (compute_complex_ancestor_jump_func): Likewise. > (compute_scalar_jump_functions): Likewise. > (compute_known_type_jump_func): Likewise, use get_ref_base_and_extent > and get_binfo_at_offset instead of get_relevant_ref_binfo. > (ipa_analyze_node): Push and pop cfun, set current_function_decl. > (update_jump_functions_after_inlining): Do not derive types from > constants. > (try_make_edge_direct_virtual_call): Likewise. > > * testsuite/g++.dg/ipa/devirt-c-1.C: New test. > * testsuite/g++.dg/ipa/devirt-c-2.C: Likewise. > * testsuite/g++.dg/ipa/devirt-c-3.C: Likewise. > * testsuite/g++.dg/ipa/devirt-c-4.C: Likewise. > * testsuite/g++.dg/ipa/devirt-c-5.C: Likewise. > * testsuite/g++.dg/ipa/devirt-d-1.C: Likewise. > * testsuite/g++.dg/torture/pr45934.C: Likewise. > > The following removals are not part of the patch but I'll svn remove > them when commiting the patch if approved: > > * testsuite/g++.dg/ipa/ipcp-ivi-1.C: Removed. > * testsuite/g++.dg/ipa/ivinline-6.C: Likewise. > > Index: icln/gcc/gimple-fold.c > =================================================================== > --- icln.orig/gcc/gimple-fold.c > +++ icln/gcc/gimple-fold.c > @@ -1360,88 +1360,6 @@ gimple_fold_builtin (gimple stmt) > return result; > } > > -/* Search for a base binfo of BINFO that corresponds to TYPE and return it if > - it is found or NULL_TREE if it is not. */ > - > -static tree > -get_base_binfo_for_type (tree binfo, tree type) > -{ > - int i; > - tree base_binfo; > - tree res = NULL_TREE; > - > - for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++) > - if (TREE_TYPE (base_binfo) == type) > - { > - gcc_assert (!res); > - res = base_binfo; > - } > - > - return res; > -} > - > -/* Return a binfo describing the part of object referenced by expression REF. > - Return NULL_TREE if it cannot be determined. REF can consist of a series of > - COMPONENT_REFs of a declaration or of an INDIRECT_REF or it can also be just > - a simple declaration, indirect reference or an SSA_NAME. If the function > - discovers an INDIRECT_REF or an SSA_NAME, it will assume that the > - encapsulating type is described by KNOWN_BINFO, if it is not NULL_TREE. > - Otherwise the first non-artificial field declaration or the base declaration > - will be examined to get the encapsulating type. */ > - > -tree > -gimple_get_relevant_ref_binfo (tree ref, tree known_binfo) > -{ > - while (true) > - { > - if (TREE_CODE (ref) == COMPONENT_REF) > - { > - tree par_type; > - tree binfo; > - tree field = TREE_OPERAND (ref, 1); > - > - if (!DECL_ARTIFICIAL (field)) > - { > - tree type = TREE_TYPE (field); > - if (TREE_CODE (type) == RECORD_TYPE) > - return TYPE_BINFO (type); > - else > - return NULL_TREE; > - } > - > - par_type = TREE_TYPE (TREE_OPERAND (ref, 0)); > - binfo = TYPE_BINFO (par_type); > - if (!binfo > - || BINFO_N_BASE_BINFOS (binfo) == 0) > - return NULL_TREE; > - > - /* Offset 0 indicates the primary base, whose vtable contents are > - represented in the binfo for the derived class. */ > - if (int_bit_position (field) != 0) > - { > - tree d_binfo; > - > - /* Get descendant binfo. */ > - d_binfo = gimple_get_relevant_ref_binfo (TREE_OPERAND (ref, 0), > - known_binfo); > - if (!d_binfo) > - return NULL_TREE; > - return get_base_binfo_for_type (d_binfo, TREE_TYPE (field)); > - } > - > - ref = TREE_OPERAND (ref, 0); > - } > - else if (DECL_P (ref) && TREE_CODE (TREE_TYPE (ref)) == RECORD_TYPE) > - return TYPE_BINFO (TREE_TYPE (ref)); > - else if (known_binfo > - && (TREE_CODE (ref) == SSA_NAME > - || TREE_CODE (ref) == MEM_REF)) > - return known_binfo; > - else > - return NULL_TREE; > - } > -} > - > /* Return a declaration of a function which an OBJ_TYPE_REF references. TOKEN > is integer form of OBJ_TYPE_REF_TOKEN of the reference expression. > KNOWN_BINFO carries the binfo describing the true type of > @@ -1525,7 +1443,7 @@ gimple_adjust_this_by_delta (gimple_stmt > INPLACE is false. Return true iff the statement was changed. */ > > static bool > -gimple_fold_obj_type_ref_call (gimple_stmt_iterator *gsi, bool inplace) > +gimple_fold_obj_type_ref_call (gimple_stmt_iterator *gsi) > { > gimple stmt = gsi_stmt (*gsi); > tree ref = gimple_call_fn (stmt); > @@ -1533,27 +1451,21 @@ gimple_fold_obj_type_ref_call (gimple_st > tree binfo, fndecl, delta; > HOST_WIDE_INT token; > > - if (TREE_CODE (obj) == ADDR_EXPR) > - obj = TREE_OPERAND (obj, 0); > - else > + if (TREE_CODE (obj) != ADDR_EXPR) > return false; > - > - binfo = gimple_get_relevant_ref_binfo (obj, NULL_TREE); > + obj = TREE_OPERAND (obj, 0); > + if (!DECL_P (obj) > + || TREE_CODE (TREE_TYPE (obj)) != RECORD_TYPE) > + return false; > + binfo = TYPE_BINFO (TREE_TYPE (obj)); > if (!binfo) > return false; > + > token = tree_low_cst (OBJ_TYPE_REF_TOKEN (ref), 1); > - fndecl = gimple_get_virt_mehtod_for_binfo (token, binfo, &delta, > - !DECL_P (obj)); > + fndecl = gimple_get_virt_mehtod_for_binfo (token, binfo, &delta, false); > if (!fndecl) > return false; > - > - if (integer_nonzerop (delta)) > - { > - if (inplace) > - return false; > - gimple_adjust_this_by_delta (gsi, delta); > - } > - > + gcc_checking_assert (integer_zerop (delta)); > gimple_call_set_fndecl (stmt, fndecl); > return true; > } > @@ -1591,7 +1503,7 @@ gimple_fold_call (gimple_stmt_iterator * > here where we can just smash the call operand. */ > callee = gimple_call_fn (stmt); > if (TREE_CODE (callee) == OBJ_TYPE_REF) > - return gimple_fold_obj_type_ref_call (gsi, inplace); > + return gimple_fold_obj_type_ref_call (gsi); > } > > return false; > Index: icln/gcc/ipa-prop.c > =================================================================== > --- icln.orig/gcc/ipa-prop.c > +++ icln/gcc/ipa-prop.c > @@ -350,6 +350,107 @@ ipa_print_all_jump_functions (FILE *f) > } > } > > +/* Helper function for detect_type_change_1 to check whether a particular > + statement is indeed an assignment to the virtual table pointer. */ > + > +static bool > +check_type_change (ao_ref *ao, tree vdef, void *data) > +{ > + gimple def_stmt = SSA_NAME_DEF_STMT (vdef); > + tree t, l, b, *res = (tree *) data; > + > + if (!is_gimple_assign (def_stmt)) > + return false; A memcpy can also be assigning to the virtual table pointer. Think of class A : public class B { virtual ... }; A *p; B *q; p = new A(); memcpy (p,q, sizeof(B)); p->xxx(); no idea if that's valid, but the middle-end could replace a pointer assignment by memcpy. That said, the is_gimple_assign check isn't conservatively correct. > + t = gimple_assign_rhs1 (def_stmt); > + if (TREE_CODE (t) != ADDR_EXPR) > + return false; Likewise. > + t = TREE_OPERAND (t, 0); > + if (TREE_CODE (t) == ARRAY_REF) > + t = TREE_OPERAND (t, 0); > + if (TREE_CODE (t) != VAR_DECL > + || !DECL_VIRTUAL_P (t)) > + return false; And it gets worse ;) You are assuming too much here. The above could be represented as vtbptr_1 = &VTBL + constant offset; MEM<&obj, vtable offset> = vtbptr_1; or in 100 other valid ways. Iff we want to recognize vtable pointer modifications we should make sure we have a single valid way to represent them, thus have special tree codes or statements for modification and access. I'm thinking of a handled-component we won't touch, like VTBL<obj, maybe we need a constant offset> = ... Of course Jason may want to comment here (esp. if we can really annotate all vtable pointer modifications and accesses this way, considering my eventually invalid testcase above). > + l = gimple_assign_lhs (def_stmt); > + while (handled_component_p (l)) > + l = TREE_OPERAND (l, 0); > + if (TREE_CODE (l) == MEM_REF) > + l = TREE_OPERAND (l, 0); > + b = ao->ref; > + while (handled_component_p (b)) > + b = TREE_OPERAND (b, 0); > + if (TREE_CODE (b) == MEM_REF) > + b = TREE_OPERAND (b, 0); > + if (l != b) > + return false; Well - see above. What you really need is for the object you want to know if its vtable is modified construct a ao_ref with base, offset and size and walk_aliased_vdefs_1 with that ref. If you ever get a callback then you are screwed and have to return true. That would be conservatively correct - but also likely not worth the effort (as in, you can as well just assume 'true' always). Note that walk_aliased_vdefs may not do what you think when walking PHIs: > + *res = DECL_CONTEXT (t); You have to consider that for if (x) store1; else store2; you can reach here for both store1 and store2 and thus have to merge *res (and have a proper state for "not mergeable"). > + return true; > +} > + > +/* Detect whether the dynamic type of ARG has changed (before callsite CALL) by > + looking for assignments to its virtual table pointer. If it is, return true > + and fill in the jump function JFUNC with relevant type information. If > + ANC_TYPE is non-NULL, look for binfo of its ancestor of that type at offset > + ANC_OFFSET. ARG is supposed to be a dereferenced pointer or a > + declaration. */ > + > +static bool > +detect_type_change_1 (tree arg, gimple call, struct ipa_jump_func *jfunc, > + tree anc_type, HOST_WIDE_INT anc_offset) > +{ > + tree res = NULL_TREE; > + ao_ref ar; > + > + /* Const calls cannot call virtual methods through VMT and so type changes do > + not matter. */ > + if (!gimple_vuse (call)) > + return false; > + ao_ref_init (&ar, arg); arg will always be a decl here? Do we always know at which offset the relevant vtable pointer is? > + walk_aliased_vdefs (&ar, gimple_vuse (call), check_type_change, &res, NULL); > + if (!res) > + return false; > + > + if (anc_type) > + { > + tree new_binfo = get_binfo_at_offset (TYPE_BINFO (res), anc_offset, > + anc_type); > + > + if (new_binfo) > + { > + jfunc->type = IPA_JF_KNOWN_TYPE; > + jfunc->value.base_binfo = new_binfo; > + } > + else > + jfunc->type = IPA_JF_UNKNOWN; > + } > + else > + { > + jfunc->type = IPA_JF_KNOWN_TYPE; > + jfunc->value.base_binfo = TYPE_BINFO (res); > + } > + > + return true; > +} > + > +/* Like detect_type_change_1 but ARG is supposed to be a non-dereferenced > + pointer SSA name. */ > + > +static bool > +detect_type_change (tree arg, gimple call, struct ipa_jump_func *jfunc, > + tree anc_type, HOST_WIDE_INT anc_offset) > +{ > + if (!POINTER_TYPE_P (TREE_TYPE (arg)) > + || TREE_CODE (TREE_TYPE (TREE_TYPE (arg))) != RECORD_TYPE) > + return false; > + gcc_checking_assert (TREE_CODE (arg) != ADDR_EXPR); > + arg = build_simple_mem_ref (arg); You definitely screw TBAA here. You want to use ao_ref_init_from_ptr_and_size instead (do you know the size of the object? remember that the type of SSA name pointers do not tell you anything about the pointed-to type. Stopping review here - I am not convinced that the above is conservatively correct (thus, you just make it harder to trigger the problems). And if you are going to detect dynamic type changes by looking at vtable pointer stores you can IMHO as well go and propagate vtable addresses instead of types and then simply do the transformation at the vtable loads instead of the calls and do devirtualization only based on constant propagation from vtable loads. For 4.6 please consider reverting back to the non-broken 4.5 state, I don't really feel comfortable with more experimenting during stage3. Thanks, Richard.
> For 4.6 please consider reverting back to the non-broken 4.5 state, > I don't really feel comfortable with more experimenting during stage3. Was 4.5 really non-broken? My understanding was that the problem with dynamic type changes was always present, just less often triggering. Honza > > Thanks, > Richard.
On Tue, 23 Nov 2010, Jan Hubicka wrote: > > For 4.6 please consider reverting back to the non-broken 4.5 state, > > I don't really feel comfortable with more experimenting during stage3. > > Was 4.5 really non-broken? My understanding was that the problem with dynamic > type changes was always present, just less often triggering. The problem of course was there, but IIRC we weren't trying to devirtualize as aggressively as we do now, no? Richard.
On 11/22/2010 05:05 PM, Martin Jambor wrote: > 2) We can never devirtualize according to a type of a global object > because we do not know whether or not the current function has been > (perhaps indirectly) called from its constructor or destructor. This > also means that we cannot devirtualize according to types of constants > propagated by IPA-CP because those are always global object. > Therefore I removed code paths deriving types from constant IPA-CP > lattices and I check that declarations are not global when extracting > BINFOs from them. > > This also means that testcases g++.dg/ipa/ipcp-ivi-1.C and > ivinline-6.C test something which cannot be safely done and therefore > I propose to remove them. In both of these testcases the call is through main; since main cannot be called directly, the call cannot be within a constructor or destructor. That may not be generally useful, though. :) Jason
Hi, On Tue, Nov 23, 2010 at 01:41:56PM +0100, Richard Guenther wrote: > On Mon, 22 Nov 2010, Martin Jambor wrote: > > > Hi, > > > > this patch fixes a wide range of issues including PR 45934 which are > > caused by the fact that the dynamic type of object changes during > > construction and destruction and devirtualization has to take these > > changes into account and has to avoid creating calls to descendants' > > implementation of a virtual functions when a constructor or a > > destructor of an ancestor is running. This is achieved by doing two > > things: > > > > 1) We cannot devirtualize OBJ_TYPE_REFs according to the static type > > of the object during folding. This code was dumbed down to what gcc > > 4.5 does, which basically devirtualizes only when the O_T_R object is > > a declaration - as opposed to an ancestor field within a declaration. > > This is necessary to make g++.dg/opt/devirt1.C pass and is safe. Note > > that we will want to fold O_T_Rs to their first parameter and we are > > currently not that far from it. > > > > Nevertheless, it also makes other testcases fail, specifically > > g++.dg/otr-fold-1.C, g++.dg/otr-fold-2.C, g++.dg/tree-ssa/pr43411.C > > and g++.dg/tree-ssa/pr45605.C. These testcases depend on type-based > > devirtualization and I will post a followup (RFC) patch that deals > > with them at -O2. > > This probably should be split out into a separate patch. Well, it was, I posted the patch separately after this one, even my paragraph above says so. Or do you mean some other part of this patch? <...> > > > 3) Whenever IPA_JF_KNOWN_TYPE, no-op IPA_JF_PASS_THROUGH or > > IPA_JF_ANCESTOR jump functions are constructed, we also walk VDEFS and > > look for preceeding code that actually changes its dynamic type by > > assigning to its virtual method table pointer. From the RHS we can > > then identify the new type of the object and construct an appropriate > > IPA_JF_KNOWN_TYPE jump function instead. > > > > This patch does not attempt to limit walking of VDEFs, I left that for > > later. I am also not quite sure how to do that. During the body scan > > that ipa-prop.c does to do modification analysis it could look for > > assignments with a RHS that looks like a VMT address (address of an > > array ref into a variable declaration which is DECL_VIRTUAL), and walk > > VDEFS only if it finds one. The potentially inefficient VDEF walking > > would then happen only in constructors, destructors and functions into > > which they were inlined by early inlining (and we could punt for big > > functions like that). > > I doubt you can conservatively identify stores to virtual method > table pointers (well, w/o identifying every store as such). You > also have to assume there is such store elsewhere if you do _not_ > find such a store. Well, I did what we agreed on in Ottawa, so I am a bit suprised that at least the general approach does not seem acceptable. Admittedly, it is basically pattern matching of what falls out of the front end for valid C++ code. > > But anyway - this part doesn't seem to be addressed in the current > patch at all, right? The efficiency issue? No. > > > This patch also replaces all remaining uses of > > gimple_get_relevant_ref_binfo with a combination of > > get_base_ref_and_extent and get_binfo_at_offset because it is actually > > convenient and it is easy to have the two binfo searching functions > > diverge even though they should not. > > It probably makes sense to split this piece into a separate patch. It would be a bit artificial thing to do and wouldn't probably make a review any easier since all callers change too. It really was very convenient to do it with the rest of this patch. > > > I assume there will be comments and suggestions. Nevertheless, the > > patch does pass bootstrap and regression tests (except for the > > testcases described above) on x86_64-linux and also make check-c++ on > > i686. Mind that it has to be applied on top of my fix for PR 46053 > > (http://gcc.gnu.org/ml/gcc-patches/2010-11/msg02291.html). > > Indeed. Comments in-line. > > > Thanks, > > > > Martin > > > > > > > > 2010-11-19 Martin Jambor <mjambor@suse.cz> > > > > PR tree-optimization/45934 > > * gimple-fold.c (get_base_binfo_for_type): Removed. > > (gimple_get_relevant_ref_binfo): Likewise. > > (gimple_fold_obj_type_ref_call): Dumb down to 4.5 functionality, > > removed parameter inplace, updated the caller. > > * ipa-cp.c (ipcp_propagate_types): Do not derive types from constants. > > (ipcp_discover_new_direct_edges): Do not do devirtualization based on > > constants. > > * gimple.h (gimple_get_relevant_ref_binfo): Remove declaration. > > * tree.c (get_binfo_at_offset): Get type from non-artificial fields. > > * ipa-prop.c (check_type_change): New function. > > (detect_type_change_1): Likewise. > > (detect_type_change): Likewise. > > (compute_complex_assign_jump_func): Check dynamic type change. > > (compute_complex_ancestor_jump_func): Likewise. > > (compute_scalar_jump_functions): Likewise. > > (compute_known_type_jump_func): Likewise, use get_ref_base_and_extent > > and get_binfo_at_offset instead of get_relevant_ref_binfo. > > (ipa_analyze_node): Push and pop cfun, set current_function_decl. > > (update_jump_functions_after_inlining): Do not derive types from > > constants. > > (try_make_edge_direct_virtual_call): Likewise. > > > > * testsuite/g++.dg/ipa/devirt-c-1.C: New test. > > * testsuite/g++.dg/ipa/devirt-c-2.C: Likewise. > > * testsuite/g++.dg/ipa/devirt-c-3.C: Likewise. > > * testsuite/g++.dg/ipa/devirt-c-4.C: Likewise. > > * testsuite/g++.dg/ipa/devirt-c-5.C: Likewise. > > * testsuite/g++.dg/ipa/devirt-d-1.C: Likewise. > > * testsuite/g++.dg/torture/pr45934.C: Likewise. > > > > The following removals are not part of the patch but I'll svn remove > > them when commiting the patch if approved: > > > > * testsuite/g++.dg/ipa/ipcp-ivi-1.C: Removed. > > * testsuite/g++.dg/ipa/ivinline-6.C: Likewise. > > > > Index: icln/gcc/gimple-fold.c > > =================================================================== > > --- icln.orig/gcc/gimple-fold.c > > +++ icln/gcc/gimple-fold.c > > @@ -1360,88 +1360,6 @@ gimple_fold_builtin (gimple stmt) > > return result; > > } > > > > -/* Search for a base binfo of BINFO that corresponds to TYPE and return it if > > - it is found or NULL_TREE if it is not. */ > > - > > -static tree > > -get_base_binfo_for_type (tree binfo, tree type) > > -{ > > - int i; > > - tree base_binfo; > > - tree res = NULL_TREE; > > - > > - for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++) > > - if (TREE_TYPE (base_binfo) == type) > > - { > > - gcc_assert (!res); > > - res = base_binfo; > > - } > > - > > - return res; > > -} > > - > > -/* Return a binfo describing the part of object referenced by expression REF. > > - Return NULL_TREE if it cannot be determined. REF can consist of a series of > > - COMPONENT_REFs of a declaration or of an INDIRECT_REF or it can also be just > > - a simple declaration, indirect reference or an SSA_NAME. If the function > > - discovers an INDIRECT_REF or an SSA_NAME, it will assume that the > > - encapsulating type is described by KNOWN_BINFO, if it is not NULL_TREE. > > - Otherwise the first non-artificial field declaration or the base declaration > > - will be examined to get the encapsulating type. */ > > - > > -tree > > -gimple_get_relevant_ref_binfo (tree ref, tree known_binfo) > > -{ > > - while (true) > > - { > > - if (TREE_CODE (ref) == COMPONENT_REF) > > - { > > - tree par_type; > > - tree binfo; > > - tree field = TREE_OPERAND (ref, 1); > > - > > - if (!DECL_ARTIFICIAL (field)) > > - { > > - tree type = TREE_TYPE (field); > > - if (TREE_CODE (type) == RECORD_TYPE) > > - return TYPE_BINFO (type); > > - else > > - return NULL_TREE; > > - } > > - > > - par_type = TREE_TYPE (TREE_OPERAND (ref, 0)); > > - binfo = TYPE_BINFO (par_type); > > - if (!binfo > > - || BINFO_N_BASE_BINFOS (binfo) == 0) > > - return NULL_TREE; > > - > > - /* Offset 0 indicates the primary base, whose vtable contents are > > - represented in the binfo for the derived class. */ > > - if (int_bit_position (field) != 0) > > - { > > - tree d_binfo; > > - > > - /* Get descendant binfo. */ > > - d_binfo = gimple_get_relevant_ref_binfo (TREE_OPERAND (ref, 0), > > - known_binfo); > > - if (!d_binfo) > > - return NULL_TREE; > > - return get_base_binfo_for_type (d_binfo, TREE_TYPE (field)); > > - } > > - > > - ref = TREE_OPERAND (ref, 0); > > - } > > - else if (DECL_P (ref) && TREE_CODE (TREE_TYPE (ref)) == RECORD_TYPE) > > - return TYPE_BINFO (TREE_TYPE (ref)); > > - else if (known_binfo > > - && (TREE_CODE (ref) == SSA_NAME > > - || TREE_CODE (ref) == MEM_REF)) > > - return known_binfo; > > - else > > - return NULL_TREE; > > - } > > -} > > - > > /* Return a declaration of a function which an OBJ_TYPE_REF references. TOKEN > > is integer form of OBJ_TYPE_REF_TOKEN of the reference expression. > > KNOWN_BINFO carries the binfo describing the true type of > > @@ -1525,7 +1443,7 @@ gimple_adjust_this_by_delta (gimple_stmt > > INPLACE is false. Return true iff the statement was changed. */ > > > > static bool > > -gimple_fold_obj_type_ref_call (gimple_stmt_iterator *gsi, bool inplace) > > +gimple_fold_obj_type_ref_call (gimple_stmt_iterator *gsi) > > { > > gimple stmt = gsi_stmt (*gsi); > > tree ref = gimple_call_fn (stmt); > > @@ -1533,27 +1451,21 @@ gimple_fold_obj_type_ref_call (gimple_st > > tree binfo, fndecl, delta; > > HOST_WIDE_INT token; > > > > - if (TREE_CODE (obj) == ADDR_EXPR) > > - obj = TREE_OPERAND (obj, 0); > > - else > > + if (TREE_CODE (obj) != ADDR_EXPR) > > return false; > > - > > - binfo = gimple_get_relevant_ref_binfo (obj, NULL_TREE); > > + obj = TREE_OPERAND (obj, 0); > > + if (!DECL_P (obj) > > + || TREE_CODE (TREE_TYPE (obj)) != RECORD_TYPE) > > + return false; > > + binfo = TYPE_BINFO (TREE_TYPE (obj)); > > if (!binfo) > > return false; > > + > > token = tree_low_cst (OBJ_TYPE_REF_TOKEN (ref), 1); > > - fndecl = gimple_get_virt_mehtod_for_binfo (token, binfo, &delta, > > - !DECL_P (obj)); > > + fndecl = gimple_get_virt_mehtod_for_binfo (token, binfo, &delta, false); > > if (!fndecl) > > return false; > > - > > - if (integer_nonzerop (delta)) > > - { > > - if (inplace) > > - return false; > > - gimple_adjust_this_by_delta (gsi, delta); > > - } > > - > > + gcc_checking_assert (integer_zerop (delta)); > > gimple_call_set_fndecl (stmt, fndecl); > > return true; > > } > > @@ -1591,7 +1503,7 @@ gimple_fold_call (gimple_stmt_iterator * > > here where we can just smash the call operand. */ > > callee = gimple_call_fn (stmt); > > if (TREE_CODE (callee) == OBJ_TYPE_REF) > > - return gimple_fold_obj_type_ref_call (gsi, inplace); > > + return gimple_fold_obj_type_ref_call (gsi); > > } > > > > return false; > > Index: icln/gcc/ipa-prop.c > > =================================================================== > > --- icln.orig/gcc/ipa-prop.c > > +++ icln/gcc/ipa-prop.c > > @@ -350,6 +350,107 @@ ipa_print_all_jump_functions (FILE *f) > > } > > } > > > > +/* Helper function for detect_type_change_1 to check whether a particular > > + statement is indeed an assignment to the virtual table pointer. */ > > + > > +static bool > > +check_type_change (ao_ref *ao, tree vdef, void *data) > > +{ > > + gimple def_stmt = SSA_NAME_DEF_STMT (vdef); > > + tree t, l, b, *res = (tree *) data; > > + > > + if (!is_gimple_assign (def_stmt)) > > + return false; > > A memcpy can also be assigning to the virtual table pointer. Think > of > > class A : public class B { virtual ... }; > > A *p; > B *q; > p = new A(); > memcpy (p,q, sizeof(B)); > p->xxx(); > > no idea if that's valid, but the middle-end could replace a > pointer assignment by memcpy. That said, the is_gimple_assign > check isn't conservatively correct. Well, I guess that memcpy could be one more thing to check for but... > > > + t = gimple_assign_rhs1 (def_stmt); > > + if (TREE_CODE (t) != ADDR_EXPR) > > + return false; > > Likewise. > > > + t = TREE_OPERAND (t, 0); > > + if (TREE_CODE (t) == ARRAY_REF) > > + t = TREE_OPERAND (t, 0); > > + if (TREE_CODE (t) != VAR_DECL > > + || !DECL_VIRTUAL_P (t)) > > + return false; > > And it gets worse ;) > > You are assuming too much here. The above could be represented as > > vtbptr_1 = &VTBL + constant offset; > MEM<&obj, vtable offset> = vtbptr_1; > > or in 100 other valid ways. > > Iff we want to recognize vtable pointer modifications we should > make sure we have a single valid way to represent them, thus > have special tree codes or statements for modification and access. > > I'm thinking of a handled-component we won't touch, like > > VTBL<obj, maybe we need a constant offset> = ... > > Of course Jason may want to comment here (esp. if we can really > annotate all vtable pointer modifications and accesses this way, > considering my eventually invalid testcase above). ... of course this would be much more invasive. > > > + l = gimple_assign_lhs (def_stmt); > > + while (handled_component_p (l)) > > + l = TREE_OPERAND (l, 0); > > + if (TREE_CODE (l) == MEM_REF) > > + l = TREE_OPERAND (l, 0); > > + b = ao->ref; > > + while (handled_component_p (b)) > > + b = TREE_OPERAND (b, 0); > > + if (TREE_CODE (b) == MEM_REF) > > + b = TREE_OPERAND (b, 0); > > + if (l != b) > > + return false; > > Well - see above. > > What you really need is for the object you want to know if its > vtable is modified construct a ao_ref with base, offset and size > and walk_aliased_vdefs_1 with that ref. If you ever get a callback > then you are screwed and have to return true. That would be > conservatively correct - but also likely not worth the effort > (as in, you can as well just assume 'true' always). > > Note that walk_aliased_vdefs may not do what you think when > walking PHIs: > > > + *res = DECL_CONTEXT (t); > > You have to consider that for > > if (x) > store1; > else > store2; > > you can reach here for both store1 and store2 and thus have to > merge *res (and have a proper state for "not mergeable"). The code is specifically targeted at C++ constructors and destructors and no other and situations such as the conditional type changes can simply never happen in those scenarios. (They would be possible with placement new but I did disregard those because those are constitute undefined code). If I understood the paragraph above that correctly, I also think it discusses situations that simply cannot happen. I specifically look for assignments only and for example intentionally disregard the possibility that the vptr is modified by a called function because such situation cannot happen or does not matter. If it was a constructor or destructor, then either the current function is a constructor/destructor of a descendant and therefore somewhere after the call (in a post-dominance sense, before any call to any function, virtual or not) there is a new assignment to all relevant vptrs, or it is a call to the top-most constructor and then the static type can be used without any problems. (The third option is that the code calls the destructor explicitely and then it does not matter because if it invokes a virtual function after the constructor finishes it is clearly bogus.) I also disregard the possibility that the vptr is modified through some alias because in constructors and destructors they are also modified through the this pointer. Yes, disregarding C++ and looking at all the thing that are theoretically possible to do in gimple, this patch is not conservatively correct. I am not quite sure whether that is really worth the effort and what we should aim for because it is of course a much more difficult problem to tackle, both in terms of development and computational complexity. > > > + return true; > > +} > > + > > +/* Detect whether the dynamic type of ARG has changed (before callsite CALL) by > > + looking for assignments to its virtual table pointer. If it is, return true > > + and fill in the jump function JFUNC with relevant type information. If > > + ANC_TYPE is non-NULL, look for binfo of its ancestor of that type at offset > > + ANC_OFFSET. ARG is supposed to be a dereferenced pointer or a > > + declaration. */ > > + > > +static bool > > +detect_type_change_1 (tree arg, gimple call, struct ipa_jump_func *jfunc, > > + tree anc_type, HOST_WIDE_INT anc_offset) > > +{ > > + tree res = NULL_TREE; > > + ao_ref ar; > > + > > + /* Const calls cannot call virtual methods through VMT and so type changes do > > + not matter. */ > > + if (!gimple_vuse (call)) > > + return false; > > + ao_ref_init (&ar, arg); > > arg will always be a decl here? It is either a declaration or a component_ref of a declaration or a dereferenced pointer (the function comment needs fixing). > Do we always know at which offset the relevant vtable pointer is? At the moment I cannot think of a situation when this would not be zero (relative to arg, not ther decl). But to be sure I'd have to try a few examples. > > > + walk_aliased_vdefs (&ar, gimple_vuse (call), check_type_change, &res, NULL); > > + if (!res) > > + return false; > > + > > + if (anc_type) > > + { > > + tree new_binfo = get_binfo_at_offset (TYPE_BINFO (res), anc_offset, > > + anc_type); > > + > > + if (new_binfo) > > + { > > + jfunc->type = IPA_JF_KNOWN_TYPE; > > + jfunc->value.base_binfo = new_binfo; > > + } > > + else > > + jfunc->type = IPA_JF_UNKNOWN; > > + } > > + else > > + { > > + jfunc->type = IPA_JF_KNOWN_TYPE; > > + jfunc->value.base_binfo = TYPE_BINFO (res); > > + } > > + > > + return true; > > +} > > + > > +/* Like detect_type_change_1 but ARG is supposed to be a non-dereferenced > > + pointer SSA name. */ > > + > > +static bool > > +detect_type_change (tree arg, gimple call, struct ipa_jump_func *jfunc, > > + tree anc_type, HOST_WIDE_INT anc_offset) > > +{ > > + if (!POINTER_TYPE_P (TREE_TYPE (arg)) > > + || TREE_CODE (TREE_TYPE (TREE_TYPE (arg))) != RECORD_TYPE) > > + return false; > > + gcc_checking_assert (TREE_CODE (arg) != ADDR_EXPR); > > + arg = build_simple_mem_ref (arg); > > You definitely screw TBAA here. You want to use > ao_ref_init_from_ptr_and_size instead I will have a look at that. > (do you know the size of the object? remember that the type of SSA > name pointers do not tell you anything about the pointed-to type. The fact that dynamic type changes in reality really means that the vptr points to a different table. The static type of the field representing ancestor is still the same during the whole lifetime of the (whole) object. Again, I am assuming no placement new is involved. > > Stopping review here - I am not convinced that the above is > conservatively correct (thus, you just make it harder to trigger > the problems). Well, I hope that I explained why I do the detection this way and disregard conditional type changes and the like. Of course, if you do not find that sufficient then I agree that devirtualization is much more difficult and even perhaps an impossible thing to do (e.g if we assume any function call can arbitrarily change the dynamic type of anything then it really isn't worth implementing, fortunately I argue that this is not what can happen). > And if you are going to detect dynamic type changes > by looking at vtable pointer stores you can IMHO as well go and > propagate vtable addresses instead of types and then simply do > the transformation at the vtable loads instead of the calls > and do devirtualization only based on constant propagation from > vtable loads. Well, let me reiterate that propagating vptrs interprocedurally is a more difficult thing to do, mainly because there is not one such pointer but one for the whole object and one for each field representing an ancestor with virtual methods. You would also need return functions containing all of them (currently we can get away without return functions rather well). And it wouldn't make the problems discussed above any simpler... in fact it would make it more difficult because we would have to catch all the assignments to vptrs not just one (and because my patch extracts the type from DECL_CONTEXT of the VMT it really does not matter which one we stumble upon first). > > For 4.6 please consider reverting back to the non-broken 4.5 state, > I don't really feel comfortable with more experimenting during stage3. If I need to introduce a new VTBL handled component then this is most probably inevitable. I still think that the code dealing with dynamic type changes should aim for dealing with the real situations only and should not need to deal with things that do not happen with valid input. Martin
Hi, On Tue, Nov 23, 2010 at 03:31:37PM +0100, Richard Guenther wrote: > On Tue, 23 Nov 2010, Jan Hubicka wrote: > > > > For 4.6 please consider reverting back to the non-broken 4.5 state, > > > I don't really feel comfortable with more experimenting during stage3. > > > > Was 4.5 really non-broken? My understanding was that the problem with dynamic > > type changes was always present, just less often triggering. > > The problem of course was there, but IIRC we weren't trying to > devirtualize as aggressively as we do now, no? 4.5 folds OBJ_TYPE_REFs only in the simplest possible scenarios (the object is a &decl), in partcular it does not attempt to fold calls to virtual methods of ancestors of the declaration (which would be represented by an object like &decl.D.1234). Therefore there are no problems with thunks or dynamic changes of types because only the ancestor part changes types. It does not check for globalness of the object so it we probably might construct a testcase that would unexpectedly fail when devirtualizing according to a global object but I believe such a testcase would be invalid because IIRC C++ standard does not allow you to access the object under creation or destruction with a type of its descendant. In short, I don't think that devirtualization in 4.5 has any problems. However I also think it is not very useeful because it works only when you have virtual methods but no inheritance. Martin
On Tue, 23 Nov 2010, Martin Jambor wrote: > Hi, > > On Tue, Nov 23, 2010 at 01:41:56PM +0100, Richard Guenther wrote: > > On Mon, 22 Nov 2010, Martin Jambor wrote: > > > > > Hi, > > > > > > this patch fixes a wide range of issues including PR 45934 which are > > > caused by the fact that the dynamic type of object changes during > > > construction and destruction and devirtualization has to take these > > > changes into account and has to avoid creating calls to descendants' > > > implementation of a virtual functions when a constructor or a > > > destructor of an ancestor is running. This is achieved by doing two > > > things: > > > > > > 1) We cannot devirtualize OBJ_TYPE_REFs according to the static type > > > of the object during folding. This code was dumbed down to what gcc > > > 4.5 does, which basically devirtualizes only when the O_T_R object is > > > a declaration - as opposed to an ancestor field within a declaration. > > > This is necessary to make g++.dg/opt/devirt1.C pass and is safe. Note > > > that we will want to fold O_T_Rs to their first parameter and we are > > > currently not that far from it. > > > > > > Nevertheless, it also makes other testcases fail, specifically > > > g++.dg/otr-fold-1.C, g++.dg/otr-fold-2.C, g++.dg/tree-ssa/pr43411.C > > > and g++.dg/tree-ssa/pr45605.C. These testcases depend on type-based > > > devirtualization and I will post a followup (RFC) patch that deals > > > with them at -O2. > > > > This probably should be split out into a separate patch. > > Well, it was, I posted the patch separately after this one, even my > paragraph above says so. Or do you mean some other part of this patch? > > <...> I probably just was confused. > > > > > 3) Whenever IPA_JF_KNOWN_TYPE, no-op IPA_JF_PASS_THROUGH or > > > IPA_JF_ANCESTOR jump functions are constructed, we also walk VDEFS and > > > look for preceeding code that actually changes its dynamic type by > > > assigning to its virtual method table pointer. From the RHS we can > > > then identify the new type of the object and construct an appropriate > > > IPA_JF_KNOWN_TYPE jump function instead. > > > > > > This patch does not attempt to limit walking of VDEFs, I left that for > > > later. I am also not quite sure how to do that. During the body scan > > > that ipa-prop.c does to do modification analysis it could look for > > > assignments with a RHS that looks like a VMT address (address of an > > > array ref into a variable declaration which is DECL_VIRTUAL), and walk > > > VDEFS only if it finds one. The potentially inefficient VDEF walking > > > would then happen only in constructors, destructors and functions into > > > which they were inlined by early inlining (and we could punt for big > > > functions like that). > > > > I doubt you can conservatively identify stores to virtual method > > table pointers (well, w/o identifying every store as such). You > > also have to assume there is such store elsewhere if you do _not_ > > find such a store. > > Well, I did what we agreed on in Ottawa, so I am a bit suprised that > at least the general approach does not seem acceptable. Admittedly, it > is basically pattern matching of what falls out of the front end for > valid C++ code. Well. I'm sort-of worried by the use of types and BINFOs by the devirtualization machinery in general. Also devirtualization happens in the middle-end and thus can't really assume it was C++ originally (ok, only ObjC seems to use OBJ_TYPE_REF and that in a weird way). That said, I'm ok with trying to fix up devirtualization for 4.6 and not drop it to 4.5 functionality. Still in general I think that not using types or binfos would be more appropriate for a generic devirtualization machinery (I'm not convinced that you can't construct a valid C++ testcase that would fail your constructor/destructor matching by just wrapping another function call - I will try to come up with a testcase for that). > > > > But anyway - this part doesn't seem to be addressed in the current > > patch at all, right? > > The efficiency issue? No. > > > > > > This patch also replaces all remaining uses of > > > gimple_get_relevant_ref_binfo with a combination of > > > get_base_ref_and_extent and get_binfo_at_offset because it is actually > > > convenient and it is easy to have the two binfo searching functions > > > diverge even though they should not. > > > > It probably makes sense to split this piece into a separate patch. > > It would be a bit artificial thing to do and wouldn't probably make a > review any easier since all callers change too. It really was very > convenient to do it with the rest of this patch. Ok. > > > > > I assume there will be comments and suggestions. Nevertheless, the > > > patch does pass bootstrap and regression tests (except for the > > > testcases described above) on x86_64-linux and also make check-c++ on > > > i686. Mind that it has to be applied on top of my fix for PR 46053 > > > (http://gcc.gnu.org/ml/gcc-patches/2010-11/msg02291.html). > > > > Indeed. Comments in-line. > > > > > Thanks, > > > > > > Martin > > > > > > > > > > > > 2010-11-19 Martin Jambor <mjambor@suse.cz> > > > > > > PR tree-optimization/45934 > > > * gimple-fold.c (get_base_binfo_for_type): Removed. > > > (gimple_get_relevant_ref_binfo): Likewise. > > > (gimple_fold_obj_type_ref_call): Dumb down to 4.5 functionality, > > > removed parameter inplace, updated the caller. > > > * ipa-cp.c (ipcp_propagate_types): Do not derive types from constants. > > > (ipcp_discover_new_direct_edges): Do not do devirtualization based on > > > constants. > > > * gimple.h (gimple_get_relevant_ref_binfo): Remove declaration. > > > * tree.c (get_binfo_at_offset): Get type from non-artificial fields. > > > * ipa-prop.c (check_type_change): New function. > > > (detect_type_change_1): Likewise. > > > (detect_type_change): Likewise. > > > (compute_complex_assign_jump_func): Check dynamic type change. > > > (compute_complex_ancestor_jump_func): Likewise. > > > (compute_scalar_jump_functions): Likewise. > > > (compute_known_type_jump_func): Likewise, use get_ref_base_and_extent > > > and get_binfo_at_offset instead of get_relevant_ref_binfo. > > > (ipa_analyze_node): Push and pop cfun, set current_function_decl. > > > (update_jump_functions_after_inlining): Do not derive types from > > > constants. > > > (try_make_edge_direct_virtual_call): Likewise. > > > > > > * testsuite/g++.dg/ipa/devirt-c-1.C: New test. > > > * testsuite/g++.dg/ipa/devirt-c-2.C: Likewise. > > > * testsuite/g++.dg/ipa/devirt-c-3.C: Likewise. > > > * testsuite/g++.dg/ipa/devirt-c-4.C: Likewise. > > > * testsuite/g++.dg/ipa/devirt-c-5.C: Likewise. > > > * testsuite/g++.dg/ipa/devirt-d-1.C: Likewise. > > > * testsuite/g++.dg/torture/pr45934.C: Likewise. > > > > > > The following removals are not part of the patch but I'll svn remove > > > them when commiting the patch if approved: > > > > > > * testsuite/g++.dg/ipa/ipcp-ivi-1.C: Removed. > > > * testsuite/g++.dg/ipa/ivinline-6.C: Likewise. > > > > > > Index: icln/gcc/gimple-fold.c > > > =================================================================== > > > --- icln.orig/gcc/gimple-fold.c > > > +++ icln/gcc/gimple-fold.c > > > @@ -1360,88 +1360,6 @@ gimple_fold_builtin (gimple stmt) > > > return result; > > > } > > > > > > -/* Search for a base binfo of BINFO that corresponds to TYPE and return it if > > > - it is found or NULL_TREE if it is not. */ > > > - > > > -static tree > > > -get_base_binfo_for_type (tree binfo, tree type) > > > -{ > > > - int i; > > > - tree base_binfo; > > > - tree res = NULL_TREE; > > > - > > > - for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++) > > > - if (TREE_TYPE (base_binfo) == type) > > > - { > > > - gcc_assert (!res); > > > - res = base_binfo; > > > - } > > > - > > > - return res; > > > -} > > > - > > > -/* Return a binfo describing the part of object referenced by expression REF. > > > - Return NULL_TREE if it cannot be determined. REF can consist of a series of > > > - COMPONENT_REFs of a declaration or of an INDIRECT_REF or it can also be just > > > - a simple declaration, indirect reference or an SSA_NAME. If the function > > > - discovers an INDIRECT_REF or an SSA_NAME, it will assume that the > > > - encapsulating type is described by KNOWN_BINFO, if it is not NULL_TREE. > > > - Otherwise the first non-artificial field declaration or the base declaration > > > - will be examined to get the encapsulating type. */ > > > - > > > -tree > > > -gimple_get_relevant_ref_binfo (tree ref, tree known_binfo) > > > -{ > > > - while (true) > > > - { > > > - if (TREE_CODE (ref) == COMPONENT_REF) > > > - { > > > - tree par_type; > > > - tree binfo; > > > - tree field = TREE_OPERAND (ref, 1); > > > - > > > - if (!DECL_ARTIFICIAL (field)) > > > - { > > > - tree type = TREE_TYPE (field); > > > - if (TREE_CODE (type) == RECORD_TYPE) > > > - return TYPE_BINFO (type); > > > - else > > > - return NULL_TREE; > > > - } > > > - > > > - par_type = TREE_TYPE (TREE_OPERAND (ref, 0)); > > > - binfo = TYPE_BINFO (par_type); > > > - if (!binfo > > > - || BINFO_N_BASE_BINFOS (binfo) == 0) > > > - return NULL_TREE; > > > - > > > - /* Offset 0 indicates the primary base, whose vtable contents are > > > - represented in the binfo for the derived class. */ > > > - if (int_bit_position (field) != 0) > > > - { > > > - tree d_binfo; > > > - > > > - /* Get descendant binfo. */ > > > - d_binfo = gimple_get_relevant_ref_binfo (TREE_OPERAND (ref, 0), > > > - known_binfo); > > > - if (!d_binfo) > > > - return NULL_TREE; > > > - return get_base_binfo_for_type (d_binfo, TREE_TYPE (field)); > > > - } > > > - > > > - ref = TREE_OPERAND (ref, 0); > > > - } > > > - else if (DECL_P (ref) && TREE_CODE (TREE_TYPE (ref)) == RECORD_TYPE) > > > - return TYPE_BINFO (TREE_TYPE (ref)); > > > - else if (known_binfo > > > - && (TREE_CODE (ref) == SSA_NAME > > > - || TREE_CODE (ref) == MEM_REF)) > > > - return known_binfo; > > > - else > > > - return NULL_TREE; > > > - } > > > -} > > > - > > > /* Return a declaration of a function which an OBJ_TYPE_REF references. TOKEN > > > is integer form of OBJ_TYPE_REF_TOKEN of the reference expression. > > > KNOWN_BINFO carries the binfo describing the true type of > > > @@ -1525,7 +1443,7 @@ gimple_adjust_this_by_delta (gimple_stmt > > > INPLACE is false. Return true iff the statement was changed. */ > > > > > > static bool > > > -gimple_fold_obj_type_ref_call (gimple_stmt_iterator *gsi, bool inplace) > > > +gimple_fold_obj_type_ref_call (gimple_stmt_iterator *gsi) > > > { > > > gimple stmt = gsi_stmt (*gsi); > > > tree ref = gimple_call_fn (stmt); > > > @@ -1533,27 +1451,21 @@ gimple_fold_obj_type_ref_call (gimple_st > > > tree binfo, fndecl, delta; > > > HOST_WIDE_INT token; > > > > > > - if (TREE_CODE (obj) == ADDR_EXPR) > > > - obj = TREE_OPERAND (obj, 0); > > > - else > > > + if (TREE_CODE (obj) != ADDR_EXPR) > > > return false; > > > - > > > - binfo = gimple_get_relevant_ref_binfo (obj, NULL_TREE); > > > + obj = TREE_OPERAND (obj, 0); > > > + if (!DECL_P (obj) > > > + || TREE_CODE (TREE_TYPE (obj)) != RECORD_TYPE) > > > + return false; > > > + binfo = TYPE_BINFO (TREE_TYPE (obj)); > > > if (!binfo) > > > return false; > > > + > > > token = tree_low_cst (OBJ_TYPE_REF_TOKEN (ref), 1); > > > - fndecl = gimple_get_virt_mehtod_for_binfo (token, binfo, &delta, > > > - !DECL_P (obj)); > > > + fndecl = gimple_get_virt_mehtod_for_binfo (token, binfo, &delta, false); > > > if (!fndecl) > > > return false; > > > - > > > - if (integer_nonzerop (delta)) > > > - { > > > - if (inplace) > > > - return false; > > > - gimple_adjust_this_by_delta (gsi, delta); > > > - } > > > - > > > + gcc_checking_assert (integer_zerop (delta)); > > > gimple_call_set_fndecl (stmt, fndecl); > > > return true; > > > } > > > @@ -1591,7 +1503,7 @@ gimple_fold_call (gimple_stmt_iterator * > > > here where we can just smash the call operand. */ > > > callee = gimple_call_fn (stmt); > > > if (TREE_CODE (callee) == OBJ_TYPE_REF) > > > - return gimple_fold_obj_type_ref_call (gsi, inplace); > > > + return gimple_fold_obj_type_ref_call (gsi); > > > } > > > > > > return false; > > > Index: icln/gcc/ipa-prop.c > > > =================================================================== > > > --- icln.orig/gcc/ipa-prop.c > > > +++ icln/gcc/ipa-prop.c > > > @@ -350,6 +350,107 @@ ipa_print_all_jump_functions (FILE *f) > > > } > > > } > > > > > > +/* Helper function for detect_type_change_1 to check whether a particular > > > + statement is indeed an assignment to the virtual table pointer. */ > > > + > > > +static bool > > > +check_type_change (ao_ref *ao, tree vdef, void *data) > > > +{ > > > + gimple def_stmt = SSA_NAME_DEF_STMT (vdef); > > > + tree t, l, b, *res = (tree *) data; > > > + > > > + if (!is_gimple_assign (def_stmt)) > > > + return false; > > > > A memcpy can also be assigning to the virtual table pointer. Think > > of > > > > class A : public class B { virtual ... }; > > > > A *p; > > B *q; > > p = new A(); > > memcpy (p,q, sizeof(B)); > > p->xxx(); > > > > no idea if that's valid, but the middle-end could replace a > > pointer assignment by memcpy. That said, the is_gimple_assign > > check isn't conservatively correct. > > Well, I guess that memcpy could be one more thing to check for but... > > > > > > + t = gimple_assign_rhs1 (def_stmt); > > > + if (TREE_CODE (t) != ADDR_EXPR) > > > + return false; > > > > Likewise. > > > > > + t = TREE_OPERAND (t, 0); > > > + if (TREE_CODE (t) == ARRAY_REF) > > > + t = TREE_OPERAND (t, 0); > > > + if (TREE_CODE (t) != VAR_DECL > > > + || !DECL_VIRTUAL_P (t)) > > > + return false; > > > > And it gets worse ;) > > > > You are assuming too much here. The above could be represented as > > > > vtbptr_1 = &VTBL + constant offset; > > MEM<&obj, vtable offset> = vtbptr_1; > > > > or in 100 other valid ways. > > > > Iff we want to recognize vtable pointer modifications we should > > make sure we have a single valid way to represent them, thus > > have special tree codes or statements for modification and access. > > > > I'm thinking of a handled-component we won't touch, like > > > > VTBL<obj, maybe we need a constant offset> = ... > > > > Of course Jason may want to comment here (esp. if we can really > > annotate all vtable pointer modifications and accesses this way, > > considering my eventually invalid testcase above). > > ... of course this would be much more invasive. Yep. I also suggest something else ... > > > > > + l = gimple_assign_lhs (def_stmt); > > > + while (handled_component_p (l)) > > > + l = TREE_OPERAND (l, 0); > > > + if (TREE_CODE (l) == MEM_REF) > > > + l = TREE_OPERAND (l, 0); > > > + b = ao->ref; > > > + while (handled_component_p (b)) > > > + b = TREE_OPERAND (b, 0); > > > + if (TREE_CODE (b) == MEM_REF) > > > + b = TREE_OPERAND (b, 0); > > > + if (l != b) > > > + return false; > > > > Well - see above. > > > > What you really need is for the object you want to know if its > > vtable is modified construct a ao_ref with base, offset and size > > and walk_aliased_vdefs_1 with that ref. If you ever get a callback > > then you are screwed and have to return true. That would be > > conservatively correct - but also likely not worth the effort > > (as in, you can as well just assume 'true' always). ... here. Basically use the aliasing infrastructure to tell if a stmt could clobber the vtbl pointer (the aliasing infrastructure is supposed to be conservatively correct). Now, if we declare virtual dispatch and devirtualization a C++ only feature (but implemented with really C++ private middle-end infrastucture) we might get away with more strict assumptions - but I know not enough of our C++ implementation details to review the pattern matching for correctness then. At least exact matching of component-refs and the like looks very fragile to my eye as it is a match that if it bogusly fails generates wrong code and not just a mere missed optimization. The matching code also needs more commentary on what you actually match. It looks like you look for (MEM<a>|a)... = &DECL; | &DECL[...]; where a is pointer equal to ao->ref base (but a can be &decl, and &decl and &decl wouldn't be the same (they are not shared)). DECL is supposedly the virtual table. I'd get to it via + t = gimple_assign_rhs1 (def_stmt); + if (TREE_CODE (t) != ADDR_EXPR) + return false; t = get_base_address (TREE_OPERAND (t, 0)); if (!t || TREE_CODE (t) != VAR_DECL || !DECL_VIRTUAL_P (t)) return false; as the array-ref could be substituted with a MEM_REF with offset for example. Likewise for the lhs I'd do + l = get_base_address (gimple_assign_lhs (def_stmt)); if (!l) return false; + if (TREE_CODE (l) == MEM_REF) + l = TREE_OPERAND (l, 0); + if (TREE_CODE (b) == MEM_REF) + b = TREE_OPERAND (b, 0); /* Now we have either a decl or an SSA name, pointer comparison ok. */ if (l != b) return false; which would at least match assignments of an address based on a virtual table somewhere into the object of ao->ref. Now - you do use that virtual table for optimization, so if I do class X : ... { virtual whatever(); void *my_fake_vtable; }; extern void *x[] __asm__(("mangled name for some vtable")); foo(X *p) { p->my_fake_vtable = &x; p->whatever(); } and have LTO merge the decls for x and the vtable then you might get confused, right? So I think you need to verify the store actually happens to the vtable slot (what about vtt table pointer stores?). Now, you need check_type_change for both correctness and optimization (and it doesn't seem to have a way to say "don't know", does it?) - either it doesn't detect a vtable store, then it presumably makes static type assumptions, or it does, in which case it uses the type of the vtable that was stored. So you need to catch both 1) all possible vtable pointer stores, 2) and only real vtable pointer stores. Thus, there's no room for errors (or being conservative to either side) in this function - which makes me feel uncomfortable. Is it possible to do only 1)? Thus, do not make use of DECL_CONTEXT of the vtable but not devirtualize in that case? > > Note that walk_aliased_vdefs may not do what you think when > > walking PHIs: > > > > > + *res = DECL_CONTEXT (t); > > > > You have to consider that for > > > > if (x) > > store1; > > else > > store2; > > > > you can reach here for both store1 and store2 and thus have to > > merge *res (and have a proper state for "not mergeable"). > > The code is specifically targeted at C++ constructors and destructors > and no other and situations such as the conditional type changes can > simply never happen in those scenarios. (They would be possible with > placement new but I did disregard those because those are constitute > undefined code). I'm always thinking of optimization passes creating this kind of situations. But well, maybe the above is not an issue (you could at least assert that *res is NULL before returning true). > If I understood the paragraph above that correctly, I also think it > discusses situations that simply cannot happen. I specifically look > for assignments only and for example intentionally disregard the > possibility that the vptr is modified by a called function because > such situation cannot happen or does not matter. If it was a > constructor or destructor, then either the current function is a > constructor/destructor of a descendant and therefore somewhere after > the call (in a post-dominance sense, before any call to any function, > virtual or not) there is a new assignment to all relevant vptrs, or it > is a call to the top-most constructor and then the static type can be > used without any problems. (The third option is that the code calls > the destructor explicitely and then it does not matter because if it > invokes a virtual function after the constructor finishes it is > clearly bogus.) I also disregard the possibility that the vptr is > modified through some alias because in constructors and destructors > they are also modified through the this pointer. Hm. I can't seem to generate a testcase that initially wraps a constructor or destructor call into a function. I considered things like copy-initialization: class A { public: virtual ~A(); virtual void foo(void); A(const A&x) { *this = x; foo(); } }; class B : public A { public: virtual ~B(); virtual void foo(void); B(const A&x); }; B::B(const A&x) : A(x) {} but at least I see ;; Function B::B(const A&) (null) ;; enabled by -tree-original { <<cleanup_point <<< Unknown tree: expr_stmt A::A (&((struct B *) this)->D.2125, (const struct A &) (const struct A *) x) >>>>>; try { <<cleanup_point <<< Unknown tree: expr_stmt (void) (((struct B *) this)->D.2125._vptr.A = &_ZTV1B + 16) >>>>>; } catch { A::~A ((struct A *) this); } thus with -fnon-call-exceptions: B::B(const A&) (struct B * const this, const struct A & x) { struct A * D.2145; <bb 2>: D.2145_2 = &this_1(D)->D.2125; # .MEM_8 = VDEF <.MEM_4(D)> D.2145_2->_vptr.A = &_ZTV1A[2]; # .MEM_9 = VDEF <.MEM_8> A::foo (D.2145_2); # .MEM_6 = VDEF <.MEM_9> this_1(D)->D.2125._vptr.A = &_ZTV1B[2]; <bb 3>: return; <L0>: # .MEM_7 = VDEF <.MEM_6> A::~A (this_1(D)); resx 1 which means that walking from A::~A you see a vtable pointer store but that actually hasn't happened when reaching the destructor (and you would devirtualize to the wrong call?). Maybe the FE can mark vtable pointer stores as non-trapping? > Yes, disregarding C++ and looking at all the thing that are > theoretically possible to do in gimple, this patch is not > conservatively correct. I am not quite sure whether that is really > worth the effort and what we should aim for because it is of course a > much more difficult problem to tackle, both in terms of development > and computational complexity. I will try to sketch what I have in mind. I'm just not sure that the situation even with C++ is as simple as it looks (just found about the EH issue above, hopefully a non-issue). Now with all of the above a way to go forward is to make check_type_change more robust (and possibly build a more precise ao_ref to walk aliases for to catch all and only vtable pointer stores). Btw, with VTT I meant virtual inheritance like class C { public: virtual ~C(); }; class A : virtual public C { public: virtual ~A(); virtual void foo(void); A(const A&x) { *this = x; foo(); } }; class B : public A, virtual public C { public: virtual ~B(); virtual void foo(void); B(const A&x); }; B::B(const A&x) : A(x) {} you get more interesting control flow and vtable pointer modifications from that (no idea if covariant returns would add even more). Also consider people testing with -fno-tree-ccp -fno-tree-forwprop (etc. ...) - using the alias oracle conservatively would just make devirtualization fail in these cases (when you detect a possible vtbl pointer store but the value stored isn't a vtable based address -- you atm just assume that it wasn't a store to the vtbl pointer). Btw, it would be nice to have a flag to disable fancy devirtualization (or to document that -fno-ipa-cp does that - does it?). Richard.
On Wed, Nov 24, 2010 at 12:09:27PM +0100, Richard Guenther wrote: > On Tue, 23 Nov 2010, Martin Jambor wrote: > > > Hi, > > > > On Tue, Nov 23, 2010 at 01:41:56PM +0100, Richard Guenther wrote: Thanks for the email. I'm having an (entirely non-technical) training today and immediately afterwards I have to leave so I will read it together with the other sub-thread and reply/act accordingly tomorrow. Martin
On Wed, 24 Nov 2010, Richard Guenther wrote: > On Tue, 23 Nov 2010, Martin Jambor wrote: > > > Hi, > > > > On Tue, Nov 23, 2010 at 01:41:56PM +0100, Richard Guenther wrote: > > > > +/* Helper function for detect_type_change_1 to check whether a particular > > > > + statement is indeed an assignment to the virtual table pointer. */ > > > > + > > > > +static bool > > > > +check_type_change (ao_ref *ao, tree vdef, void *data) > > > > +{ > > > > + gimple def_stmt = SSA_NAME_DEF_STMT (vdef); > > > > + tree t, l, b, *res = (tree *) data; > > > > + > > > > + if (!is_gimple_assign (def_stmt)) > > > > + return false; > > > > > > A memcpy can also be assigning to the virtual table pointer. Think > > > of > > > > > > class A : public class B { virtual ... }; > > > > > > A *p; > > > B *q; > > > p = new A(); > > > memcpy (p,q, sizeof(B)); > > > p->xxx(); > > > > > > no idea if that's valid, but the middle-end could replace a > > > pointer assignment by memcpy. That said, the is_gimple_assign > > > check isn't conservatively correct. > > > > Well, I guess that memcpy could be one more thing to check for but... > > > > > > > > > + t = gimple_assign_rhs1 (def_stmt); > > > > + if (TREE_CODE (t) != ADDR_EXPR) > > > > + return false; > > > > > > Likewise. > > > > > > > + t = TREE_OPERAND (t, 0); > > > > + if (TREE_CODE (t) == ARRAY_REF) > > > > + t = TREE_OPERAND (t, 0); > > > > + if (TREE_CODE (t) != VAR_DECL > > > > + || !DECL_VIRTUAL_P (t)) > > > > + return false; > > > > > > And it gets worse ;) > > > > > > You are assuming too much here. The above could be represented as > > > > > > vtbptr_1 = &VTBL + constant offset; > > > MEM<&obj, vtable offset> = vtbptr_1; > > > > > > or in 100 other valid ways. > > > > > > Iff we want to recognize vtable pointer modifications we should > > > make sure we have a single valid way to represent them, thus > > > have special tree codes or statements for modification and access. > > > > > > I'm thinking of a handled-component we won't touch, like > > > > > > VTBL<obj, maybe we need a constant offset> = ... > > > > > > Of course Jason may want to comment here (esp. if we can really > > > annotate all vtable pointer modifications and accesses this way, > > > considering my eventually invalid testcase above). > > > > ... of course this would be much more invasive. > > Yep. I also suggest something else ... > > > > > > > > + l = gimple_assign_lhs (def_stmt); > > > > + while (handled_component_p (l)) > > > > + l = TREE_OPERAND (l, 0); > > > > + if (TREE_CODE (l) == MEM_REF) > > > > + l = TREE_OPERAND (l, 0); > > > > + b = ao->ref; > > > > + while (handled_component_p (b)) > > > > + b = TREE_OPERAND (b, 0); > > > > + if (TREE_CODE (b) == MEM_REF) > > > > + b = TREE_OPERAND (b, 0); > > > > + if (l != b) > > > > + return false; > > > > > > Well - see above. > > > > > > What you really need is for the object you want to know if its > > > vtable is modified construct a ao_ref with base, offset and size > > > and walk_aliased_vdefs_1 with that ref. If you ever get a callback > > > then you are screwed and have to return true. That would be > > > conservatively correct - but also likely not worth the effort > > > (as in, you can as well just assume 'true' always). > > ... here. Basically use the aliasing infrastructure to tell > if a stmt could clobber the vtbl pointer (the aliasing infrastructure > is supposed to be conservatively correct). > > Now, if we declare virtual dispatch and devirtualization a C++ only > feature (but implemented with really C++ private middle-end infrastucture) > we might get away with more strict assumptions - but I know not > enough of our C++ implementation details to review the pattern > matching for correctness then. At least exact matching of > component-refs and the like looks very fragile to my eye as it > is a match that if it bogusly fails generates wrong code and > not just a mere missed optimization. The matching code also > needs more commentary on what you actually match. It looks > like you look for > > (MEM<a>|a)... = &DECL; | &DECL[...]; > > where a is pointer equal to ao->ref base (but a can be &decl, > and &decl and &decl wouldn't be the same (they are not shared)). > > DECL is supposedly the virtual table. I'd get to it via > > + t = gimple_assign_rhs1 (def_stmt); > + if (TREE_CODE (t) != ADDR_EXPR) > + return false; > t = get_base_address (TREE_OPERAND (t, 0)); > if (!t || TREE_CODE (t) != VAR_DECL || !DECL_VIRTUAL_P (t)) > return false; > > as the array-ref could be substituted with a MEM_REF with offset > for example. > > Likewise for the lhs I'd do > > + l = get_base_address (gimple_assign_lhs (def_stmt)); > if (!l) > return false; > + if (TREE_CODE (l) == MEM_REF) > + l = TREE_OPERAND (l, 0); > + if (TREE_CODE (b) == MEM_REF) > + b = TREE_OPERAND (b, 0); > /* Now we have either a decl or an SSA name, pointer comparison ok. */ > if (l != b) > return false; > > which would at least match assignments of an address based on a > virtual table somewhere into the object of ao->ref. > > Now - you do use that virtual table for optimization, so if I do > > class X : ... { > virtual whatever(); > void *my_fake_vtable; > }; > extern void *x[] __asm__(("mangled name for some vtable")); > foo(X *p) > { > p->my_fake_vtable = &x; > p->whatever(); > } > > and have LTO merge the decls for x and the vtable then you might > get confused, right? > > So I think you need to verify the store actually happens to the > vtable slot (what about vtt table pointer stores?). > > Now, you need check_type_change for both correctness and > optimization (and it doesn't seem to have a way to say "don't know", > does it?) - either it doesn't detect a vtable store, then it > presumably makes static type assumptions, or it does, in which > case it uses the type of the vtable that was stored. So you > need to catch both 1) all possible vtable pointer stores, > 2) and only real vtable pointer stores. Thus, there's no > room for errors (or being conservative to either side) in this > function - which makes me feel uncomfortable. Is it possible > to do only 1)? Thus, do not make use of DECL_CONTEXT of the > vtable but not devirtualize in that case? To followup myself here - the following maybe clarifies the above. You have to split check_type_change, one part, bool stmt_may_be_vtbl_ptr_store (gimple stmt) is never allowed to return false if the stmt is a vtbl pointer store. A trivially correct implementation is just 'return true;'. The other part, bool stmt_must_be_vtbl_ptr_store (gimple stmt, tree *vtbl) is never allowed to return true if the stmt is _not_ a vtbl pointer store or the stored vtbl cannot be determined. A trivially correct implementation is just 'return false'. If stmt_may_be_vtbl_ptr_store returns true and stmt_must_be_vtbl_ptr_store returns false you cannot devirtualize (and thus cannot create a jump function). Your current implementation does not allow for this case. The stmt_may_be_vtbl_ptr_store can be implemented by just walk_aliased_vdefs if the provided reference is correct. > > > Note that walk_aliased_vdefs may not do what you think when > > > walking PHIs: > > > > > > > + *res = DECL_CONTEXT (t); > > > > > > You have to consider that for > > > > > > if (x) > > > store1; > > > else > > > store2; > > > > > > you can reach here for both store1 and store2 and thus have to > > > merge *res (and have a proper state for "not mergeable"). > > > > The code is specifically targeted at C++ constructors and destructors > > and no other and situations such as the conditional type changes can > > simply never happen in those scenarios. (They would be possible with > > placement new but I did disregard those because those are constitute > > undefined code). > > I'm always thinking of optimization passes creating this kind > of situations. But well, maybe the above is not an issue (you > could at least assert that *res is NULL before returning true). > > > If I understood the paragraph above that correctly, I also think it > > discusses situations that simply cannot happen. I specifically look > > for assignments only and for example intentionally disregard the > > possibility that the vptr is modified by a called function because > > such situation cannot happen or does not matter. If it was a > > constructor or destructor, then either the current function is a > > constructor/destructor of a descendant and therefore somewhere after > > the call (in a post-dominance sense, before any call to any function, > > virtual or not) there is a new assignment to all relevant vptrs, or it > > is a call to the top-most constructor and then the static type can be > > used without any problems. (The third option is that the code calls > > the destructor explicitely and then it does not matter because if it > > invokes a virtual function after the constructor finishes it is > > clearly bogus.) I also disregard the possibility that the vptr is > > modified through some alias because in constructors and destructors > > they are also modified through the this pointer. > > Hm. I can't seem to generate a testcase that initially wraps > a constructor or destructor call into a function. But of course function splitting can trivially create such a case. Richard.
On Nov 24, 2010, at 3:09 AM, Richard Guenther wrote: > Well. I'm sort-of worried by the use of types and BINFOs by the > devirtualization machinery in general. It should be controlled and considered. An architect should consider the best way of doing the optimization. I think having the optimization seems reasonable. > Also devirtualization happens in the middle-end and thus can't really > assume it was C++ originally (ok, only ObjC seems to use OBJ_TYPE_REF > and that in a weird way). The shapes used by the devirtualization machinery needs to be documented in the middle end and needs to be appropriate given how C++ and Objective C use those shapes. Neither C++ nor Objective C can then change to violate the requirements of the middle end and should C++ change to not follow the shapes as documented, the optimization would just stop applying. In practice I don't see this as such a big deal.
On 11/22/2010 02:05 PM, Martin Jambor wrote: > 2) We can never devirtualize according to a type of a global object > because we do not know whether or not the current function has been > (perhaps indirectly) called from its constructor or destructor. This > also means that we cannot devirtualize according to types of constants > propagated by IPA-CP because those are always global object. > Therefore I removed code paths deriving types from constant IPA-CP > lattices and I check that declarations are not global when extracting > BINFOs from them. Surely at least with LTO we have enough call graph available to determine that a function F, referencing global object G, is not called within the constructor of G. I expect that many constructors will not call functions external to the module at all, at which point the call-graph contains all the path information between the constructors and F. The constructor for G might call an external, non-"leaf" function. At which point we can check whether F is reachable from any entry point into the module. r~
Index: icln/gcc/gimple-fold.c =================================================================== --- icln.orig/gcc/gimple-fold.c +++ icln/gcc/gimple-fold.c @@ -1360,88 +1360,6 @@ gimple_fold_builtin (gimple stmt) return result; } -/* Search for a base binfo of BINFO that corresponds to TYPE and return it if - it is found or NULL_TREE if it is not. */ - -static tree -get_base_binfo_for_type (tree binfo, tree type) -{ - int i; - tree base_binfo; - tree res = NULL_TREE; - - for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++) - if (TREE_TYPE (base_binfo) == type) - { - gcc_assert (!res); - res = base_binfo; - } - - return res; -} - -/* Return a binfo describing the part of object referenced by expression REF. - Return NULL_TREE if it cannot be determined. REF can consist of a series of - COMPONENT_REFs of a declaration or of an INDIRECT_REF or it can also be just - a simple declaration, indirect reference or an SSA_NAME. If the function - discovers an INDIRECT_REF or an SSA_NAME, it will assume that the - encapsulating type is described by KNOWN_BINFO, if it is not NULL_TREE. - Otherwise the first non-artificial field declaration or the base declaration - will be examined to get the encapsulating type. */ - -tree -gimple_get_relevant_ref_binfo (tree ref, tree known_binfo) -{ - while (true) - { - if (TREE_CODE (ref) == COMPONENT_REF) - { - tree par_type; - tree binfo; - tree field = TREE_OPERAND (ref, 1); - - if (!DECL_ARTIFICIAL (field)) - { - tree type = TREE_TYPE (field); - if (TREE_CODE (type) == RECORD_TYPE) - return TYPE_BINFO (type); - else - return NULL_TREE; - } - - par_type = TREE_TYPE (TREE_OPERAND (ref, 0)); - binfo = TYPE_BINFO (par_type); - if (!binfo - || BINFO_N_BASE_BINFOS (binfo) == 0) - return NULL_TREE; - - /* Offset 0 indicates the primary base, whose vtable contents are - represented in the binfo for the derived class. */ - if (int_bit_position (field) != 0) - { - tree d_binfo; - - /* Get descendant binfo. */ - d_binfo = gimple_get_relevant_ref_binfo (TREE_OPERAND (ref, 0), - known_binfo); - if (!d_binfo) - return NULL_TREE; - return get_base_binfo_for_type (d_binfo, TREE_TYPE (field)); - } - - ref = TREE_OPERAND (ref, 0); - } - else if (DECL_P (ref) && TREE_CODE (TREE_TYPE (ref)) == RECORD_TYPE) - return TYPE_BINFO (TREE_TYPE (ref)); - else if (known_binfo - && (TREE_CODE (ref) == SSA_NAME - || TREE_CODE (ref) == MEM_REF)) - return known_binfo; - else - return NULL_TREE; - } -} - /* Return a declaration of a function which an OBJ_TYPE_REF references. TOKEN is integer form of OBJ_TYPE_REF_TOKEN of the reference expression. KNOWN_BINFO carries the binfo describing the true type of @@ -1525,7 +1443,7 @@ gimple_adjust_this_by_delta (gimple_stmt INPLACE is false. Return true iff the statement was changed. */ static bool -gimple_fold_obj_type_ref_call (gimple_stmt_iterator *gsi, bool inplace) +gimple_fold_obj_type_ref_call (gimple_stmt_iterator *gsi) { gimple stmt = gsi_stmt (*gsi); tree ref = gimple_call_fn (stmt); @@ -1533,27 +1451,21 @@ gimple_fold_obj_type_ref_call (gimple_st tree binfo, fndecl, delta; HOST_WIDE_INT token; - if (TREE_CODE (obj) == ADDR_EXPR) - obj = TREE_OPERAND (obj, 0); - else + if (TREE_CODE (obj) != ADDR_EXPR) return false; - - binfo = gimple_get_relevant_ref_binfo (obj, NULL_TREE); + obj = TREE_OPERAND (obj, 0); + if (!DECL_P (obj) + || TREE_CODE (TREE_TYPE (obj)) != RECORD_TYPE) + return false; + binfo = TYPE_BINFO (TREE_TYPE (obj)); if (!binfo) return false; + token = tree_low_cst (OBJ_TYPE_REF_TOKEN (ref), 1); - fndecl = gimple_get_virt_mehtod_for_binfo (token, binfo, &delta, - !DECL_P (obj)); + fndecl = gimple_get_virt_mehtod_for_binfo (token, binfo, &delta, false); if (!fndecl) return false; - - if (integer_nonzerop (delta)) - { - if (inplace) - return false; - gimple_adjust_this_by_delta (gsi, delta); - } - + gcc_checking_assert (integer_zerop (delta)); gimple_call_set_fndecl (stmt, fndecl); return true; } @@ -1591,7 +1503,7 @@ gimple_fold_call (gimple_stmt_iterator * here where we can just smash the call operand. */ callee = gimple_call_fn (stmt); if (TREE_CODE (callee) == OBJ_TYPE_REF) - return gimple_fold_obj_type_ref_call (gsi, inplace); + return gimple_fold_obj_type_ref_call (gsi); } return false; Index: icln/gcc/ipa-prop.c =================================================================== --- icln.orig/gcc/ipa-prop.c +++ icln/gcc/ipa-prop.c @@ -350,6 +350,107 @@ ipa_print_all_jump_functions (FILE *f) } } +/* Helper function for detect_type_change_1 to check whether a particular + statement is indeed an assignment to the virtual table pointer. */ + +static bool +check_type_change (ao_ref *ao, tree vdef, void *data) +{ + gimple def_stmt = SSA_NAME_DEF_STMT (vdef); + tree t, l, b, *res = (tree *) data; + + if (!is_gimple_assign (def_stmt)) + return false; + + t = gimple_assign_rhs1 (def_stmt); + if (TREE_CODE (t) != ADDR_EXPR) + return false; + t = TREE_OPERAND (t, 0); + if (TREE_CODE (t) == ARRAY_REF) + t = TREE_OPERAND (t, 0); + if (TREE_CODE (t) != VAR_DECL + || !DECL_VIRTUAL_P (t)) + return false; + + l = gimple_assign_lhs (def_stmt); + while (handled_component_p (l)) + l = TREE_OPERAND (l, 0); + if (TREE_CODE (l) == MEM_REF) + l = TREE_OPERAND (l, 0); + b = ao->ref; + while (handled_component_p (b)) + b = TREE_OPERAND (b, 0); + if (TREE_CODE (b) == MEM_REF) + b = TREE_OPERAND (b, 0); + if (l != b) + return false; + + *res = DECL_CONTEXT (t); + return true; +} + +/* Detect whether the dynamic type of ARG has changed (before callsite CALL) by + looking for assignments to its virtual table pointer. If it is, return true + and fill in the jump function JFUNC with relevant type information. If + ANC_TYPE is non-NULL, look for binfo of its ancestor of that type at offset + ANC_OFFSET. ARG is supposed to be a dereferenced pointer or a + declaration. */ + +static bool +detect_type_change_1 (tree arg, gimple call, struct ipa_jump_func *jfunc, + tree anc_type, HOST_WIDE_INT anc_offset) +{ + tree res = NULL_TREE; + ao_ref ar; + + /* Const calls cannot call virtual methods through VMT and so type changes do + not matter. */ + if (!gimple_vuse (call)) + return false; + ao_ref_init (&ar, arg); + walk_aliased_vdefs (&ar, gimple_vuse (call), check_type_change, &res, NULL); + if (!res) + return false; + + if (anc_type) + { + tree new_binfo = get_binfo_at_offset (TYPE_BINFO (res), anc_offset, + anc_type); + + if (new_binfo) + { + jfunc->type = IPA_JF_KNOWN_TYPE; + jfunc->value.base_binfo = new_binfo; + } + else + jfunc->type = IPA_JF_UNKNOWN; + } + else + { + jfunc->type = IPA_JF_KNOWN_TYPE; + jfunc->value.base_binfo = TYPE_BINFO (res); + } + + return true; +} + +/* Like detect_type_change_1 but ARG is supposed to be a non-dereferenced + pointer SSA name. */ + +static bool +detect_type_change (tree arg, gimple call, struct ipa_jump_func *jfunc, + tree anc_type, HOST_WIDE_INT anc_offset) +{ + if (!POINTER_TYPE_P (TREE_TYPE (arg)) + || TREE_CODE (TREE_TYPE (TREE_TYPE (arg))) != RECORD_TYPE) + return false; + gcc_checking_assert (TREE_CODE (arg) != ADDR_EXPR); + arg = build_simple_mem_ref (arg); + + return detect_type_change_1 (arg, call, jfunc, anc_type, anc_offset); +} + + /* Given that an actual argument is an SSA_NAME (given in NAME) and is a result of an assignment statement STMT, try to find out whether NAME can be described by a (possibly polynomial) pass-through jump-function or an @@ -359,10 +460,10 @@ ipa_print_all_jump_functions (FILE *f) static void compute_complex_assign_jump_func (struct ipa_node_params *info, struct ipa_jump_func *jfunc, - gimple stmt, tree name) + gimple call, gimple stmt, tree name) { HOST_WIDE_INT offset, size, max_size; - tree op1, op2, type; + tree op1, op2, base, type; int index; op1 = gimple_assign_rhs1 (stmt); @@ -388,7 +489,8 @@ compute_complex_assign_jump_func (struct jfunc->value.pass_through.operation = gimple_assign_rhs_code (stmt); jfunc->value.pass_through.operand = op2; } - else if (gimple_assign_unary_nop_p (stmt)) + else if (gimple_assign_unary_nop_p (stmt) + && !detect_type_change (op1, call, jfunc, NULL_TREE, 0)) { jfunc->type = IPA_JF_PASS_THROUGH; jfunc->value.pass_through.formal_id = index; @@ -404,21 +506,22 @@ compute_complex_assign_jump_func (struct type = TREE_TYPE (op1); if (TREE_CODE (type) != RECORD_TYPE) return; - op1 = get_ref_base_and_extent (op1, &offset, &size, &max_size); - if (TREE_CODE (op1) != MEM_REF + base = get_ref_base_and_extent (op1, &offset, &size, &max_size); + if (TREE_CODE (base) != MEM_REF /* If this is a varying address, punt. */ || max_size == -1 || max_size != size) return; - offset += mem_ref_offset (op1).low * BITS_PER_UNIT; - op1 = TREE_OPERAND (op1, 0); - if (TREE_CODE (op1) != SSA_NAME - || !SSA_NAME_IS_DEFAULT_DEF (op1) + offset += mem_ref_offset (base).low * BITS_PER_UNIT; + base = TREE_OPERAND (base, 0); + if (TREE_CODE (base) != SSA_NAME + || !SSA_NAME_IS_DEFAULT_DEF (base) || offset < 0) return; - index = ipa_get_param_decl_index (info, SSA_NAME_VAR (op1)); - if (index >= 0) + /* Dynamic types are changed only in constructors and destructors and */ + index = ipa_get_param_decl_index (info, SSA_NAME_VAR (base)); + if (index >= 0 && !detect_type_change_1 (op1, call, jfunc, type, offset)) { jfunc->type = IPA_JF_ANCESTOR; jfunc->value.ancestor.formal_id = index; @@ -452,19 +555,23 @@ compute_complex_assign_jump_func (struct static void compute_complex_ancestor_jump_func (struct ipa_node_params *info, struct ipa_jump_func *jfunc, - gimple phi) + gimple call, gimple phi) { HOST_WIDE_INT offset, size, max_size; gimple assign, cond; basic_block phi_bb, assign_bb, cond_bb; - tree tmp, parm, expr; + tree tmp, parm, expr, anc_type; int index, i; - if (gimple_phi_num_args (phi) != 2 - || !integer_zerop (PHI_ARG_DEF (phi, 1))) + if (gimple_phi_num_args (phi) != 2) return; - tmp = PHI_ARG_DEF (phi, 0); + if (integer_zerop (PHI_ARG_DEF (phi, 1))) + tmp = PHI_ARG_DEF (phi, 0); + else if (integer_zerop (PHI_ARG_DEF (phi, 0))) + tmp = PHI_ARG_DEF (phi, 1); + else + return; if (TREE_CODE (tmp) != SSA_NAME || SSA_NAME_IS_DEFAULT_DEF (tmp) || !POINTER_TYPE_P (TREE_TYPE (tmp)) @@ -508,7 +615,6 @@ compute_complex_ancestor_jump_func (stru || !integer_zerop (gimple_cond_rhs (cond))) return; - phi_bb = gimple_bb (phi); for (i = 0; i < 2; i++) { @@ -517,10 +623,14 @@ compute_complex_ancestor_jump_func (stru return; } - jfunc->type = IPA_JF_ANCESTOR; - jfunc->value.ancestor.formal_id = index; - jfunc->value.ancestor.offset = offset; - jfunc->value.ancestor.type = TREE_TYPE (TREE_TYPE (tmp)); + anc_type = TREE_TYPE (TREE_TYPE (tmp)); + if (!detect_type_change (parm, call, jfunc, anc_type, offset)) + { + jfunc->type = IPA_JF_ANCESTOR; + jfunc->value.ancestor.formal_id = index; + jfunc->value.ancestor.offset = offset; + jfunc->value.ancestor.type = anc_type; + } } /* Given OP whch is passed as an actual argument to a called function, @@ -528,15 +638,32 @@ compute_complex_ancestor_jump_func (stru and if so, create one and store it to JFUNC. */ static void -compute_known_type_jump_func (tree op, struct ipa_jump_func *jfunc) +compute_known_type_jump_func (tree op, struct ipa_jump_func *jfunc, + gimple call) { - tree binfo; + HOST_WIDE_INT offset, size, max_size; + tree base, binfo; - if (TREE_CODE (op) != ADDR_EXPR) + if (TREE_CODE (op) != ADDR_EXPR + || TREE_CODE (TREE_TYPE (TREE_TYPE (op))) != RECORD_TYPE) return; op = TREE_OPERAND (op, 0); - binfo = gimple_get_relevant_ref_binfo (op, NULL_TREE); + base = get_ref_base_and_extent (op, &offset, &size, &max_size); + if (!DECL_P (base) + || max_size == -1 + || max_size != size + || TREE_CODE (TREE_TYPE (base)) != RECORD_TYPE + || is_global_var (base)) + return; + + if (detect_type_change_1 (op, call, jfunc, TREE_TYPE (op), offset)) + return; + + binfo = TYPE_BINFO (TREE_TYPE (base)); + if (!binfo) + return; + binfo = get_binfo_at_offset (binfo, offset, TREE_TYPE (op)); if (binfo) { jfunc->type = IPA_JF_KNOWN_TYPE; @@ -574,7 +701,9 @@ compute_scalar_jump_functions (struct ip { int index = ipa_get_param_decl_index (info, SSA_NAME_VAR (arg)); - if (index >= 0) + if (index >= 0 + && !detect_type_change (arg, call, &functions[num], + NULL_TREE, 0)) { functions[num].type = IPA_JF_PASS_THROUGH; functions[num].value.pass_through.formal_id = index; @@ -586,14 +715,14 @@ compute_scalar_jump_functions (struct ip gimple stmt = SSA_NAME_DEF_STMT (arg); if (is_gimple_assign (stmt)) compute_complex_assign_jump_func (info, &functions[num], - stmt, arg); + call, stmt, arg); else if (gimple_code (stmt) == GIMPLE_PHI) compute_complex_ancestor_jump_func (info, &functions[num], - stmt); + call, stmt); } } else - compute_known_type_jump_func (arg, &functions[num]); + compute_known_type_jump_func (arg, &functions[num], call); } } @@ -1346,6 +1475,9 @@ ipa_analyze_node (struct cgraph_node *no struct param_analysis_info *parms_info; int i, param_count; + push_cfun (DECL_STRUCT_FUNCTION (node->decl)); + current_function_decl = node->decl; + ipa_initialize_node_params (node); param_count = ipa_get_param_count (info); @@ -1358,6 +1490,9 @@ ipa_analyze_node (struct cgraph_node *no for (i = 0; i < param_count; i++) if (parms_info[i].visited_statements) BITMAP_FREE (parms_info[i].visited_statements); + + current_function_decl = NULL; + pop_cfun (); } @@ -1416,17 +1551,6 @@ update_jump_functions_after_inlining (st src = ipa_get_ith_jump_func (top, dst->value.ancestor.formal_id); if (src->type == IPA_JF_KNOWN_TYPE) combine_known_type_and_ancestor_jfs (src, dst); - else if (src->type == IPA_JF_CONST) - { - struct ipa_jump_func kt_func; - - kt_func.type = IPA_JF_UNKNOWN; - compute_known_type_jump_func (src->value.constant, &kt_func); - if (kt_func.type == IPA_JF_KNOWN_TYPE) - combine_known_type_and_ancestor_jfs (&kt_func, dst); - else - dst->type = IPA_JF_UNKNOWN; - } else if (src->type == IPA_JF_PASS_THROUGH && src->value.pass_through.operation == NOP_EXPR) dst->value.ancestor.formal_id = src->value.pass_through.formal_id; @@ -1539,15 +1663,6 @@ try_make_edge_direct_virtual_call (struc if (jfunc->type == IPA_JF_KNOWN_TYPE) binfo = jfunc->value.base_binfo; - else if (jfunc->type == IPA_JF_CONST) - { - tree cst = jfunc->value.constant; - if (TREE_CODE (cst) == ADDR_EXPR) - binfo = gimple_get_relevant_ref_binfo (TREE_OPERAND (cst, 0), - NULL_TREE); - else - return NULL; - } else return NULL; Index: icln/gcc/testsuite/g++.dg/ipa/devirt-c-1.C =================================================================== --- /dev/null +++ icln/gcc/testsuite/g++.dg/ipa/devirt-c-1.C @@ -0,0 +1,76 @@ +/* Verify that ipa-cp correctly detects the dynamic type of an object + under construction when doing devirtualization. */ +/* { dg-do run } */ +/* { dg-options "-O3 -fno-early-inlining -fno-inline -fdump-ipa-cp -fdump-tree-optimized" } */ + +extern "C" void abort (void); + +class A +{ +public: + int data; + A(); + virtual int foo (int i); +}; + +class B : public A +{ +public: + virtual int foo (int i); +}; + +class C : public A +{ +public: + virtual int foo (int i); +}; + +int A::foo (int i) +{ + return i + 1; +} + +int B::foo (int i) +{ + return i + 2; +} + +int C::foo (int i) +{ + return i + 3; +} + +static int middleman (class A *obj, int i) +{ + return obj->foo (i); +} + +int __attribute__ ((noinline,noclone)) get_input(void) +{ + return 1; +} + +A::A () +{ + if (middleman (this, get_input ()) != 2) + abort (); +} + +static void bah () +{ + class B b; +} + +int main (int argc, char *argv[]) +{ + int i; + + for (i = 0; i < 10; i++) + bah (); + return 0; +} + +/* { dg-final { scan-ipa-dump "Discovered a virtual call to a known target.*A::foo" "cp" } } */ +/* { dg-final { scan-tree-dump-times "OBJ_TYPE_REF" 0 "optimized"} } */ +/* { dg-final { cleanup-ipa-dump "cp" } } */ +/* { dg-final { cleanup-tree-dump "optimized" } } */ Index: icln/gcc/testsuite/g++.dg/ipa/devirt-c-2.C =================================================================== --- /dev/null +++ icln/gcc/testsuite/g++.dg/ipa/devirt-c-2.C @@ -0,0 +1,84 @@ +/* Verify that ipa-cp correctly detects the dynamic type of an object + under construction when doing devirtualization. */ +/* { dg-do run } */ +/* { dg-options "-O3 -fno-early-inlining -fno-inline -fdump-ipa-cp -fdump-tree-optimized" } */ + +extern "C" void abort (void); + +class Distraction +{ +public: + float f; + double d; + Distraction () + { + f = 8.3; + d = 10.2; + } + virtual float bar (float z); +}; + +class A +{ +public: + int data; + A(); + virtual int foo (int i); +}; + +class B : public Distraction, public A +{ +public: + virtual int foo (int i); +}; + +float Distraction::bar (float z) +{ + f += z; + return f/2; +} + +int A::foo (int i) +{ + return i + 1; +} + +int B::foo (int i) +{ + return i + 2; +} + +int __attribute__ ((noinline,noclone)) get_input(void) +{ + return 1; +} + +static int middleman (class A *obj, int i) +{ + return obj->foo (i); +} + +A::A() +{ + if (middleman (this, get_input ()) != 2) + abort (); +} + +static void bah () +{ + class B b; +} + +int main (int argc, char *argv[]) +{ + int i; + + for (i = 0; i < 10; i++) + bah (); + return 0; +} + +/* { dg-final { scan-ipa-dump "Discovered a virtual call to a known target.*A::foo" "cp" } } */ +/* { dg-final { scan-tree-dump-times "OBJ_TYPE_REF" 0 "optimized"} } */ +/* { dg-final { cleanup-ipa-dump "cp" } } */ +/* { dg-final { cleanup-tree-dump "optimized" } } */ Index: icln/gcc/testsuite/g++.dg/ipa/devirt-c-3.C =================================================================== --- /dev/null +++ icln/gcc/testsuite/g++.dg/ipa/devirt-c-3.C @@ -0,0 +1,85 @@ +/* Verify that ipa-cp correctly detects the dynamic type of an object + under construction when doing devirtualization. */ +/* { dg-do run } */ +/* { dg-options "-O3 -fno-inline -fdump-ipa-cp -fdump-tree-optimized" } */ + +extern "C" void abort (void); + +class Distraction +{ +public: + float f; + double d; + Distraction () + { + f = 8.3; + d = 10.2; + } + virtual float bar (float z); +}; + +class A +{ +public: + int data; + A(); + virtual int foo (int i); +}; + +class B : public Distraction, public A +{ +public: + virtual int foo (int i); +}; + +float Distraction::bar (float z) +{ + f += z; + return f/2; +} + +int A::foo (int i) +{ + return i + 1; +} + +int B::foo (int i) +{ + return i + 2; +} + +int __attribute__ ((noinline,noclone)) get_input(void) +{ + return 1; +} + +static int __attribute__ ((noinline)) +middleman (class A *obj, int i) +{ + return obj->foo (i); +} + +inline __attribute__ ((always_inline)) A::A() +{ + if (middleman (this, get_input ()) != 2) + abort (); +} + +static void bah () +{ + class B b; +} + +int main (int argc, char *argv[]) +{ + int i; + + for (i = 0; i < 10; i++) + bah (); + return 0; +} + +/* { dg-final { scan-ipa-dump "Discovered a virtual call to a known target.*A::foo" "cp" } } */ +/* { dg-final { scan-tree-dump-times "OBJ_TYPE_REF" 0 "optimized"} } */ +/* { dg-final { cleanup-ipa-dump "cp" } } */ +/* { dg-final { cleanup-tree-dump "optimized" } } */ Index: icln/gcc/testsuite/g++.dg/ipa/devirt-c-4.C =================================================================== --- /dev/null +++ icln/gcc/testsuite/g++.dg/ipa/devirt-c-4.C @@ -0,0 +1,113 @@ +/* Verify that ipa-cp correctly detects the dynamic type of an object + under construction when doing devirtualization. */ +/* { dg-do run } */ +/* { dg-options "-O3 -fno-inline -fdump-tree-optimized" } */ + +extern "C" void abort (void); + +class Distraction +{ +public: + float f; + double d; + Distraction () + { + f = 8.3; + d = 10.2; + } + virtual float bar (float z); +}; + +class A +{ +public: + int data; + A(); + virtual int foo (int i); +}; + +class B : public Distraction, public A +{ +public: + B(); + virtual int foo (int i); +}; + +class C : public B +{ +public: + virtual int foo (int i); +}; + + +float Distraction::bar (float z) +{ + f += z; + return f/2; +} + +int A::foo (int i) +{ + return i + 1; +} + +int B::foo (int i) +{ + return i + 2; +} + +int C::foo (int i) +{ + return i + 3; +} + +int __attribute__ ((noinline,noclone)) get_input(void) +{ + return 1; +} + +static int __attribute__ ((noinline)) +middleman (class A *obj, int i) +{ + return obj->foo (i); +} + +static void __attribute__ ((noinline)) +sth2 (A *a) +{ + if (a->foo (get_input ()) != 3) + abort (); +} + +inline void __attribute__ ((always_inline)) sth1 (B *b) +{ + sth2 (b); +} + +inline __attribute__ ((always_inline)) A::A() +{ + if (middleman (this, get_input ()) != 2) + abort (); +} + +B::B() : Distraction(), A() +{ + sth1 (this); +} + +static void bah () +{ + class C c; +} + +int main (int argc, char *argv[]) +{ + int i; + + for (i = 0; i < 10; i++) + bah (); + return 0; +} + +/* { dg-final { scan-tree-dump-times "OBJ_TYPE_REF" 0 "optimized"} } */ +/* { dg-final { cleanup-tree-dump "optimized" } } */ Index: icln/gcc/testsuite/g++.dg/ipa/devirt-d-1.C =================================================================== --- /dev/null +++ icln/gcc/testsuite/g++.dg/ipa/devirt-d-1.C @@ -0,0 +1,76 @@ +/* Verify that ipa-cp correctly detects the dynamic type of an object + under destruction when doing devirtualization. */ +/* { dg-do run } */ +/* { dg-options "-O3 -fno-early-inlining -fno-inline -fdump-ipa-cp -fdump-tree-optimized" } */ + +extern "C" void abort (void); + +class A +{ +public: + int data; + ~A(); + virtual int foo (int i); +}; + +class B : public A +{ +public: + virtual int foo (int i); +}; + +class C : public A +{ +public: + virtual int foo (int i); +}; + +int A::foo (int i) +{ + return i + 1; +} + +int B::foo (int i) +{ + return i + 2; +} + +int C::foo (int i) +{ + return i + 3; +} + +static int middleman (class A *obj, int i) +{ + return obj->foo (i); +} + +int __attribute__ ((noinline,noclone)) get_input(void) +{ + return 1; +} + +A::~A () +{ + if (middleman (this, get_input ()) != 2) + abort (); +} + +static void bah () +{ + class B b; +} + +int main (int argc, char *argv[]) +{ + int i; + + for (i = 0; i < 10; i++) + bah (); + return 0; +} + +/* { dg-final { scan-ipa-dump "Discovered a virtual call to a known target.*A::foo" "cp" } } */ +/* { dg-final { scan-tree-dump-times "OBJ_TYPE_REF" 0 "optimized"} } */ +/* { dg-final { cleanup-ipa-dump "cp" } } */ +/* { dg-final { cleanup-tree-dump "optimized" } } */ Index: icln/gcc/testsuite/g++.dg/torture/pr45934.C =================================================================== --- /dev/null +++ icln/gcc/testsuite/g++.dg/torture/pr45934.C @@ -0,0 +1,23 @@ +/* { dg-do run } */ + +extern "C" void abort (); + +struct B *b; + +struct B +{ + virtual void f () { } + ~B() { b->f(); } +}; + +struct D : public B +{ + virtual void f () { abort (); } +}; + +int main () +{ + D d; + b = &d; + return 0; +} Index: icln/gcc/tree.c =================================================================== --- icln.orig/gcc/tree.c +++ icln/gcc/tree.c @@ -10949,8 +10949,7 @@ get_binfo_at_offset (tree binfo, HOST_WI if (type == expected_type) return binfo; - if (TREE_CODE (type) != RECORD_TYPE - || offset < 0) + if (offset < 0) return NULL_TREE; for (fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld)) @@ -10963,12 +10962,18 @@ get_binfo_at_offset (tree binfo, HOST_WI if (pos <= offset && (pos + size) > offset) break; } - if (!fld || !DECL_ARTIFICIAL (fld)) + if (!fld || TREE_CODE (TREE_TYPE (fld)) != RECORD_TYPE) return NULL_TREE; + if (!DECL_ARTIFICIAL (fld)) + { + binfo = TYPE_BINFO (TREE_TYPE (fld)); + if (!binfo) + return NULL_TREE; + } /* Offset 0 indicates the primary base, whose vtable contents are represented in the binfo for the derived class. */ - if (offset != 0) + else if (offset != 0) { tree base_binfo, found_binfo = NULL_TREE; for (i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++) Index: icln/gcc/gimple.h =================================================================== --- icln.orig/gcc/gimple.h +++ icln/gcc/gimple.h @@ -895,7 +895,6 @@ unsigned get_gimple_rhs_num_ops (enum tr gimple gimple_alloc_stat (enum gimple_code, unsigned MEM_STAT_DECL); const char *gimple_decl_printable_name (tree, int); bool gimple_fold_call (gimple_stmt_iterator *gsi, bool inplace); -tree gimple_get_relevant_ref_binfo (tree ref, tree known_binfo); tree gimple_get_virt_mehtod_for_binfo (HOST_WIDE_INT, tree, tree *, bool); void gimple_adjust_this_by_delta (gimple_stmt_iterator *, tree); /* Returns true iff T is a valid GIMPLE statement. */ Index: icln/gcc/ipa-cp.c =================================================================== --- icln.orig/gcc/ipa-cp.c +++ icln/gcc/ipa-cp.c @@ -781,26 +781,16 @@ ipcp_propagate_types (struct ipa_node_pa struct ipa_node_params *callee_info, struct ipa_jump_func *jf, int i) { - tree cst, binfo; - switch (jf->type) { case IPA_JF_UNKNOWN: case IPA_JF_CONST_MEMBER_PTR: + case IPA_JF_CONST: break; case IPA_JF_KNOWN_TYPE: return ipcp_add_param_type (callee_info, i, jf->value.base_binfo); - case IPA_JF_CONST: - cst = jf->value.constant; - if (TREE_CODE (cst) != ADDR_EXPR) - break; - binfo = gimple_get_relevant_ref_binfo (TREE_OPERAND (cst, 0), NULL_TREE); - if (!binfo) - break; - return ipcp_add_param_type (callee_info, i, binfo); - case IPA_JF_PASS_THROUGH: case IPA_JF_ANCESTOR: return ipcp_copy_types (caller_info, callee_info, i, jf); @@ -1292,35 +1282,13 @@ ipcp_discover_new_direct_edges (struct c for (ie = node->indirect_calls; ie; ie = next_ie) { struct cgraph_indirect_call_info *ici = ie->indirect_info; - tree target, delta = NULL_TREE; next_ie = ie->next_callee; - if (ici->param_index != index) + if (ici->param_index != index + || ici->polymorphic) continue; - if (ici->polymorphic) - { - tree binfo; - HOST_WIDE_INT token; - - if (TREE_CODE (cst) != ADDR_EXPR) - continue; - - binfo = gimple_get_relevant_ref_binfo (TREE_OPERAND (cst, 0), - NULL_TREE); - if (!binfo) - continue; - gcc_assert (ie->indirect_info->anc_offset == 0); - token = ie->indirect_info->otr_token; - target = gimple_get_virt_mehtod_for_binfo (token, binfo, &delta, - true); - if (!target) - continue; - } - else - target = cst; - - ipa_make_edge_direct_to_target (ie, target, delta); + ipa_make_edge_direct_to_target (ie, cst, NULL_TREE); } } Index: icln/gcc/testsuite/g++.dg/ipa/devirt-c-5.C =================================================================== --- /dev/null +++ icln/gcc/testsuite/g++.dg/ipa/devirt-c-5.C @@ -0,0 +1,82 @@ +/* Verify that ipa-cp correctly detects the dynamic type of an object + under construction when doing devirtualization. */ +/* { dg-do run } */ +/* { dg-options "-O3 -fno-early-inlining -fno-inline -fdump-ipa-cp" } */ + +extern "C" void abort (void); + +class B; + +class A +{ +public: + int data; + A(); + A(B *b); + virtual int foo (int i); +}; + +class B : public A +{ +public: + virtual int foo (int i); +}; + +class C : public A +{ +public: + virtual int foo (int i); +}; + +int A::foo (int i) +{ + return i + 1; +} + +int B::foo (int i) +{ + return i + 2; +} + +int C::foo (int i) +{ + return i + 3; +} + +static int middleman (class A *obj, int i) +{ + return obj->foo (i); +} + +int __attribute__ ((noinline,noclone)) get_input(void) +{ + return 1; +} + +A::A () +{ +} + +A::A (B *b) +{ + if (middleman (b, get_input ()) != 3) + abort (); +} + +static void bah () +{ + B b; + A a(&b); +} + +int main (int argc, char *argv[]) +{ + int i; + + for (i = 0; i < 10; i++) + bah (); + return 0; +} + +/* { dg-final { scan-ipa-dump "Discovered a virtual call to a known target.*B::foo" "cp" } } */ +/* { dg-final { cleanup-ipa-dump "cp" } } */