| Message ID | 20170310150718.xeg374b2z3lgicon@virgil.suse.cz |
|---|---|
| State | New |
| Headers | show |
On Fri, 10 Mar 2017, Martin Jambor wrote: > Hi, > > PR 77333 is a i686-windows target bug, which however has its root in > our general mechanism of adjusting gimple statements when redirecting > call graph edge. Basically, these three things trigger it: > > 1) IPA-CP figures out that the this parameter of a C++ class method is > unused and because the class is in an anonymous namespace, it can > be removed and all calls adjusted. That effectively changes a > normal method into a static method and so internally, its type > changes from METHOD_TYPE to FUNCTION_TYPE. > > 2) Since the fix of PR 57330, we do not update gimple_call_fntype to > match the new type, in fact we explicitely set it to the old, now > invalid, type (see redirect_call_stmt_to_callee in cgraph.c). > > 3) Function ix86_get_callcvt which decides on call ABI, ends with the > following condition: > > if (ret != 0 > || is_stdarg > || TREE_CODE (type) != METHOD_TYPE > || ix86_function_type_abi (type) != MS_ABI) > return IX86_CALLCVT_CDECL | ret; > > return IX86_CALLCVT_THISCALL; > > ...and since now the callee is no longer a METHOD_TYPE but callers > still think that they are, leading to calling convention mismatches > and subsequent crashes. It took me quite a lot of time to come up > with a small testcase (reproducible using wine) but eventually I > managed. > > The fix is not to do 2) above, but doing so without re-introducing PR > 57330, of course. There are two options. One is to use the > call_stmt_cannot_inline_p flag of call-graph edges and not do any > IPA-CP accross those edges. That is done in the patch below. The (so > far a bit hypothetical) problem with that approach is that the call > graph edge flag may not be 100% reliable in LTO, because incompatible > decls might get merged and then we wold re-introduce PR 57330 again - > only with on invalid code and with LTO but an ICE nevertheless. So you mean replacing a matching decl with a non-matching and thus during symtab merge introduce the issue. You could detect whenever merging incompatible symbols and set ->cannot_change_signature on the prevailing node though? But how do we deal with devirt here? That is propagation itself may introduce (knowledge of) the incompatibility... In general I am sympathetic with not doing any IPA propagation across call stmt signature incompatibilties. Of course we may be still too strict in those compatibility check... > So the alternative would be to re-check when doing the gimple > statement adjustment and if the types match, then set the correct new > gimple_fntype and if they don't... then we can either leave it be or > just run the same type transformation on it as we did on the callee, > though they would be bogus either way. That is implemented in the > attached patch. As we _do_ adjust the call (apply the transform to its actual arguments) we should apply the same transform to gimple_call_fntype. We can avoid doing duplicate work in case the old fndecl (do we still have that around?) matched fntype. I think the common case of "mismatches" is having them with respect to the "extern" declaration but then the implementation is actually ok (otherwise people would run into runtime issues). I wonder what we do to IPA-SRA-kind of transforms ... (as it's early it's much easier to just not do anything about those at this point). > I have successfully bootstrapped both patches on x86_64-linux and I > have also tested them both on a windows cross-compiler and wine (with > some noise but I believe it is just noise). > > Honza, Richi, do you prefer any one approach over the other? > Actually, we can have both, would that be desirable? I think I'd like to see statistics for say, SPEC, how many cgraph edges we disable transforms for. For correctness I'd simply do the same transform to fntype as to the actual stmt (which of course has some cost). Richard. > Thanks, > > Martin > > > 2017-03-02 Martin Jambor <mjambor@suse.cz> > > PR ipa/77333 > * ipa-prop.h (ipa_node_params): New field call_stmt_type_mismatch. > (ipa_node_params::ipa_node_params): Initialize it to zero. > * cgraph.c (redirect_call_stmt_to_callee): Set gimple fntype to > the type of the new target. > * ipa-cp.c (propagate_constants_across_call): Set variable flag of > lattices and call_stmt_type_mismatch of the callee when > encountering an edge with mismatched types. > (estimate_local_effects): Do not clone for all contexts when > call_stmt_type_mismatch is set. > > testsuite/ > * g++.dg/ipa/pr77333.C: New test. > --- > gcc/cgraph.c | 2 +- > gcc/ipa-cp.c | 11 ++++--- > gcc/ipa-prop.h | 4 ++- > gcc/testsuite/g++.dg/ipa/pr77333.C | 65 ++++++++++++++++++++++++++++++++++++++ > 4 files changed, 76 insertions(+), 6 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/ipa/pr77333.C > > diff --git a/gcc/cgraph.c b/gcc/cgraph.c > index 839388496ee..642ff0bcfc2 100644 > --- a/gcc/cgraph.c > +++ b/gcc/cgraph.c > @@ -1425,7 +1425,7 @@ cgraph_edge::redirect_call_stmt_to_callee (void) > new_stmt = chkp_copy_call_skip_bounds (new_stmt); > > gimple_call_set_fndecl (new_stmt, e->callee->decl); > - gimple_call_set_fntype (new_stmt, gimple_call_fntype (e->call_stmt)); > + gimple_call_set_fntype (new_stmt, TREE_TYPE (e->callee->decl)); > > if (gimple_vdef (new_stmt) > && TREE_CODE (gimple_vdef (new_stmt)) == SSA_NAME) > diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c > index aa3c9973a66..d27151ffade 100644 > --- a/gcc/ipa-cp.c > +++ b/gcc/ipa-cp.c > @@ -2231,9 +2231,11 @@ propagate_constants_across_call (struct cgraph_edge *cs) > checking instrumentation_clone flag for chain source and target. > Going through instrumentation thunks we always have it changed > from 0 to 1 and all other nodes do not change it. */ > - if (!cs->callee->instrumentation_clone > - && callee->instrumentation_clone) > + if (cs->call_stmt_cannot_inline_p > + || (!cs->callee->instrumentation_clone > + && callee->instrumentation_clone)) > { > + callee_info->call_stmt_type_mismatch = true; > for (i = 0; i < parms_count; i++) > ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info, > i)); > @@ -2841,8 +2843,9 @@ estimate_local_effects (struct cgraph_node *node) > known_aggs_ptrs = agg_jmp_p_vec_for_t_vec (known_aggs); > int devirt_bonus = devirtualization_time_bonus (node, known_csts, > known_contexts, known_aggs_ptrs); > - if (always_const || devirt_bonus > - || (removable_params_cost && node->local.can_change_signature)) > + if (!info->call_stmt_type_mismatch > + && (always_const || devirt_bonus > + || (removable_params_cost && node->local.can_change_signature))) > { > struct caller_statistics stats; > inline_hints hints; > diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h > index 8f7eb088813..612268415ff 100644 > --- a/gcc/ipa-prop.h > +++ b/gcc/ipa-prop.h > @@ -360,6 +360,8 @@ struct GTY((for_user)) ipa_node_params > unsigned node_calling_single_call : 1; > /* False when there is something makes versioning impossible. */ > unsigned versionable : 1; > + /* Node is called with a call statement with mismatched types. */ > + unsigned call_stmt_type_mismatch : 1; > }; > > inline > @@ -368,7 +370,7 @@ ipa_node_params::ipa_node_params () > known_csts (vNULL), known_contexts (vNULL), analysis_done (0), > node_enqueued (0), do_clone_for_all_contexts (0), is_all_contexts_clone (0), > node_dead (0), node_within_scc (0), node_calling_single_call (0), > - versionable (0) > + versionable (0), call_stmt_type_mismatch (0) > { > } > > diff --git a/gcc/testsuite/g++.dg/ipa/pr77333.C b/gcc/testsuite/g++.dg/ipa/pr77333.C > new file mode 100644 > index 00000000000..1ef997f7a54 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ipa/pr77333.C > @@ -0,0 +1,65 @@ > +// { dg-do run } > +// { dg-options "-O2 -fno-ipa-sra" } > + > +volatile int global; > +int __attribute__((noinline, noclone)) > +get_data (int i) > +{ > + global = i; > + return i; > +} > + > +typedef int array[32]; > + > +namespace { > + > +char buf[512]; > + > +class A > +{ > +public: > + int field; > + char *s; > + > + A() : field(223344) > + { > + s = buf; > + } > + > + int __attribute__((noinline)) > + foo (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, > + int k, int l, int m, int n, int o, int p, int q, int r, int s, int t) > + { > + global = a+b+c+d+e+f+g+h+i+j+k+l+m+n+o+p+q+r+s+t; > + return global; > + } > + > + int __attribute__((noinline)) > + bar() > + { > + int r = foo (get_data (1), get_data (1), get_data (1), get_data (1), > + get_data (1), get_data (1), get_data (1), get_data (1), > + get_data (1), get_data (1), get_data (1), get_data (1), > + get_data (1), get_data (1), get_data (1), get_data (1), > + get_data (1), get_data (1), get_data (1), get_data (1)); > + > + if (field != 223344) > + __builtin_abort (); > + return 0; > + } > +}; > + > +} > + > +int main (int argc, char **argv) > +{ > + A a; > + int r = a.bar(); > + r = a.bar (); > + if (a.field != 223344) > + __builtin_abort (); > + if (global != 20) > + __builtin_abort (); > + > + return r; > +} >
Hi, On Mon, Mar 13, 2017 at 01:46:47PM +0100, Richard Biener wrote: > On Fri, 10 Mar 2017, Martin Jambor wrote: > > > Hi, > > > > PR 77333 is a i686-windows target bug, which however has its root in > > our general mechanism of adjusting gimple statements when redirecting > > call graph edge. Basically, these three things trigger it: > > > > 1) IPA-CP figures out that the this parameter of a C++ class method is > > unused and because the class is in an anonymous namespace, it can > > be removed and all calls adjusted. That effectively changes a > > normal method into a static method and so internally, its type > > changes from METHOD_TYPE to FUNCTION_TYPE. > > > > 2) Since the fix of PR 57330, we do not update gimple_call_fntype to > > match the new type, in fact we explicitely set it to the old, now > > invalid, type (see redirect_call_stmt_to_callee in cgraph.c). > > > > 3) Function ix86_get_callcvt which decides on call ABI, ends with the > > following condition: > > > > if (ret != 0 > > || is_stdarg > > || TREE_CODE (type) != METHOD_TYPE > > || ix86_function_type_abi (type) != MS_ABI) > > return IX86_CALLCVT_CDECL | ret; > > > > return IX86_CALLCVT_THISCALL; > > > > ...and since now the callee is no longer a METHOD_TYPE but callers > > still think that they are, leading to calling convention mismatches > > and subsequent crashes. It took me quite a lot of time to come up > > with a small testcase (reproducible using wine) but eventually I > > managed. > > > > The fix is not to do 2) above, but doing so without re-introducing PR > > 57330, of course. There are two options. One is to use the > > call_stmt_cannot_inline_p flag of call-graph edges and not do any > > IPA-CP accross those edges. That is done in the patch below. The (so > > far a bit hypothetical) problem with that approach is that the call > > graph edge flag may not be 100% reliable in LTO, because incompatible > > decls might get merged and then we wold re-introduce PR 57330 again - > > only with on invalid code and with LTO but an ICE nevertheless. > > So you mean replacing a matching decl with a non-matching and thus > during symtab merge introduce the issue. You could detect whenever > merging incompatible symbols and set ->cannot_change_signature on > the prevailing node though? > > But how do we deal with devirt here? That is propagation itself > may introduce (knowledge of) the incompatibility... True, I haven't thought about that. > > In general I am sympathetic with not doing any IPA propagation > across call stmt signature incompatibilties. Of course we may > be still too strict in those compatibility check... > > > So the alternative would be to re-check when doing the gimple > > statement adjustment and if the types match, then set the correct new > > gimple_fntype and if they don't... then we can either leave it be or > > just run the same type transformation on it as we did on the callee, > > though they would be bogus either way. That is implemented in the > > attached patch. > > As we _do_ adjust the call (apply the transform to its actual > arguments) we should apply the same transform to > gimple_call_fntype. We can avoid doing duplicate work in case > the old fndecl (do we still have that around?) matched fntype. > > I think the common case of "mismatches" is having them with > respect to the "extern" declaration but then the implementation > is actually ok (otherwise people would run into runtime issues). > > I wonder what we do to IPA-SRA-kind of transforms ... (as it's early > it's much easier to just not do anything about those at this point). > > > I have successfully bootstrapped both patches on x86_64-linux and I > > have also tested them both on a windows cross-compiler and wine (with > > some noise but I believe it is just noise). > > > > Honza, Richi, do you prefer any one approach over the other? > > Actually, we can have both, would that be desirable? > > I think I'd like to see statistics for say, SPEC, how many > cgraph edges we disable transforms for. I used some old SPEC 2k6 config file, which meant that 400.perlbench, 416.gamess, 447.dealII and 450.soplex failed for some reason in normal build and 400.perlbench, 416.gamess, 435.gromacs, 436.cactusADM, 447.dealII, 450.soplex, 454.calculix in the LTO-build. In non-LTO build we did not seem to disable anything, in LTO one however, surprisingly many: Number of function clonings performed by IPA-CP: | Build | Unpatched | Patched | |---------+-----------+---------| | non-LTO | 1598 | 1598 | | LTO | 5489 | 4881 | > For correctness I'd simply > do the same transform to fntype as to the actual stmt (which of course > has some cost). > After talking to Honza today, we decided to probably go this route and use the patch doing the type conversion at acall-sites when necessary. Honza promised to review the patch soon (he wants to figure out why former_clone_of can be NULL, something I decided not to bother about since at that time I thought the other approach was going to be preferable). Thanks, Martin
[PR 77333] Fixup fntypes of gimple calls of clones 2017-03-02 Martin Jambor <mjambor@suse.cz> PR ipa/77333 * cgraph.h (cgraph_build_function_type_skip_args): Declare. * cgraph.c (redirect_call_stmt_to_callee): Set gimple fntype so that it reflects the signature changes performed at the callee side. * cgraphclones.c (build_function_type_skip_args): Make public, renamed to cgraph_build_function_type_skip_args. (build_function_decl_skip_args): Adjust call to the above function. testsuite/ * g++.dg/ipa/pr77333.C: New test. --- gcc/cgraph.c | 16 +++++++++- gcc/cgraph.h | 2 ++ gcc/cgraphclones.c | 9 +++--- gcc/testsuite/g++.dg/ipa/pr77333.C | 65 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/g++.dg/ipa/pr77333.C diff --git a/gcc/cgraph.c b/gcc/cgraph.c index 839388496ee..5143eafa2a6 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -1424,8 +1424,22 @@ cgraph_edge::redirect_call_stmt_to_callee (void) if (skip_bounds) new_stmt = chkp_copy_call_skip_bounds (new_stmt); + tree old_fntype = gimple_call_fntype (e->call_stmt); + tree old_fndecl = gimple_call_fndecl (e->call_stmt); gimple_call_set_fndecl (new_stmt, e->callee->decl); - gimple_call_set_fntype (new_stmt, gimple_call_fntype (e->call_stmt)); + if ((e->callee->former_clone_of + && old_fntype == TREE_TYPE (e->callee->former_clone_of)) + || (old_fndecl + && gimple_check_call_matching_types (e->call_stmt, + old_fndecl, false))) + gimple_call_set_fntype (new_stmt, TREE_TYPE (e->callee->decl)); + else + { + bitmap skip = e->callee->clone.combined_args_to_skip; + tree t = cgraph_build_function_type_skip_args (old_fntype, skip, + false); + gimple_call_set_fntype (new_stmt, t); + } if (gimple_vdef (new_stmt) && TREE_CODE (gimple_vdef (new_stmt)) == SSA_NAME) diff --git a/gcc/cgraph.h b/gcc/cgraph.h index 3889a3e1701..62cebd9e55a 100644 --- a/gcc/cgraph.h +++ b/gcc/cgraph.h @@ -2326,6 +2326,8 @@ void tree_function_versioning (tree, tree, vec<ipa_replace_map *, va_gc> *, void dump_callgraph_transformation (const cgraph_node *original, const cgraph_node *clone, const char *suffix); +tree cgraph_build_function_type_skip_args (tree orig_type, bitmap args_to_skip, + bool skip_return); /* In cgraphbuild.c */ int compute_call_stmt_bb_frequency (tree, basic_block bb); diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c index c2337e84553..69572b926c4 100644 --- a/gcc/cgraphclones.c +++ b/gcc/cgraphclones.c @@ -152,9 +152,9 @@ cgraph_edge::clone (cgraph_node *n, gcall *call_stmt, unsigned stmt_uid, /* Build variant of function type ORIG_TYPE skipping ARGS_TO_SKIP and the return value if SKIP_RETURN is true. */ -static tree -build_function_type_skip_args (tree orig_type, bitmap args_to_skip, - bool skip_return) +tree +cgraph_build_function_type_skip_args (tree orig_type, bitmap args_to_skip, + bool skip_return) { tree new_type = NULL; tree args, new_args = NULL; @@ -219,7 +219,8 @@ build_function_decl_skip_args (tree orig_decl, bitmap args_to_skip, if (prototype_p (new_type) || (skip_return && !VOID_TYPE_P (TREE_TYPE (new_type)))) new_type - = build_function_type_skip_args (new_type, args_to_skip, skip_return); + = cgraph_build_function_type_skip_args (new_type, args_to_skip, + skip_return); TREE_TYPE (new_decl) = new_type; /* For declarations setting DECL_VINDEX (i.e. methods) diff --git a/gcc/testsuite/g++.dg/ipa/pr77333.C b/gcc/testsuite/g++.dg/ipa/pr77333.C new file mode 100644 index 00000000000..1ef997f7a54 --- /dev/null +++ b/gcc/testsuite/g++.dg/ipa/pr77333.C @@ -0,0 +1,65 @@ +// { dg-do run } +// { dg-options "-O2 -fno-ipa-sra" } + +volatile int global; +int __attribute__((noinline, noclone)) +get_data (int i) +{ + global = i; + return i; +} + +typedef int array[32]; + +namespace { + +char buf[512]; + +class A +{ +public: + int field; + char *s; + + A() : field(223344) + { + s = buf; + } + + int __attribute__((noinline)) + foo (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, + int k, int l, int m, int n, int o, int p, int q, int r, int s, int t) + { + global = a+b+c+d+e+f+g+h+i+j+k+l+m+n+o+p+q+r+s+t; + return global; + } + + int __attribute__((noinline)) + bar() + { + int r = foo (get_data (1), get_data (1), get_data (1), get_data (1), + get_data (1), get_data (1), get_data (1), get_data (1), + get_data (1), get_data (1), get_data (1), get_data (1), + get_data (1), get_data (1), get_data (1), get_data (1), + get_data (1), get_data (1), get_data (1), get_data (1)); + + if (field != 223344) + __builtin_abort (); + return 0; + } +}; + +} + +int main (int argc, char **argv) +{ + A a; + int r = a.bar(); + r = a.bar (); + if (a.field != 223344) + __builtin_abort (); + if (global != 20) + __builtin_abort (); + + return r; +} -- 2.12.0
Hi, PR 77333 is a i686-windows target bug, which however has its root in our general mechanism of adjusting gimple statements when redirecting call graph edge. Basically, these three things trigger it: 1) IPA-CP figures out that the this parameter of a C++ class method is unused and because the class is in an anonymous namespace, it can be removed and all calls adjusted. That effectively changes a normal method into a static method and so internally, its type changes from METHOD_TYPE to FUNCTION_TYPE. 2) Since the fix of PR 57330, we do not update gimple_call_fntype to match the new type, in fact we explicitely set it to the old, now invalid, type (see redirect_call_stmt_to_callee in cgraph.c). 3) Function ix86_get_callcvt which decides on call ABI, ends with the following condition: if (ret != 0 || is_stdarg || TREE_CODE (type) != METHOD_TYPE || ix86_function_type_abi (type) != MS_ABI) return IX86_CALLCVT_CDECL | ret; return IX86_CALLCVT_THISCALL; ...and since now the callee is no longer a METHOD_TYPE but callers still think that they are, leading to calling convention mismatches and subsequent crashes. It took me quite a lot of time to come up with a small testcase (reproducible using wine) but eventually I managed. The fix is not to do 2) above, but doing so without re-introducing PR 57330, of course. There are two options. One is to use the call_stmt_cannot_inline_p flag of call-graph edges and not do any IPA-CP accross those edges. That is done in the patch below. The (so far a bit hypothetical) problem with that approach is that the call graph edge flag may not be 100% reliable in LTO, because incompatible decls might get merged and then we wold re-introduce PR 57330 again - only with on invalid code and with LTO but an ICE nevertheless. So the alternative would be to re-check when doing the gimple statement adjustment and if the types match, then set the correct new gimple_fntype and if they don't... then we can either leave it be or just run the same type transformation on it as we did on the callee, though they would be bogus either way. That is implemented in the attached patch. I have successfully bootstrapped both patches on x86_64-linux and I have also tested them both on a windows cross-compiler and wine (with some noise but I believe it is just noise). Honza, Richi, do you prefer any one approach over the other? Actually, we can have both, would that be desirable? Thanks, Martin 2017-03-02 Martin Jambor <mjambor@suse.cz> PR ipa/77333 * ipa-prop.h (ipa_node_params): New field call_stmt_type_mismatch. (ipa_node_params::ipa_node_params): Initialize it to zero. * cgraph.c (redirect_call_stmt_to_callee): Set gimple fntype to the type of the new target. * ipa-cp.c (propagate_constants_across_call): Set variable flag of lattices and call_stmt_type_mismatch of the callee when encountering an edge with mismatched types. (estimate_local_effects): Do not clone for all contexts when call_stmt_type_mismatch is set. testsuite/ * g++.dg/ipa/pr77333.C: New test. --- gcc/cgraph.c | 2 +- gcc/ipa-cp.c | 11 ++++--- gcc/ipa-prop.h | 4 ++- gcc/testsuite/g++.dg/ipa/pr77333.C | 65 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/g++.dg/ipa/pr77333.C