| Message ID | ri6mudfod1z.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | [PR,70929] Remove gimple_call_types_likely_match_p... almost | expand |
On Fri, 1 Nov 2019, Martin Jambor wrote: > Hi, > > I have spent some more time looking into PR 70929, how > gimple_check_call_matching_types behaves when LTO-building Firefox to > see what could replace it or if we perhaps could remove it. > > TL;DR: > I believe it can and should be removed, possibly except the use in > value-prof.c where I replaced it with a clearly imprecise predicate to > catch cases where the profile info is corrupted and since I had it, I > also ended up using it for speculative devirtualization, in case it got > its guess somehow wrong (but I have not seen either of the two cases > happening). See the patch below. > > > More details: > With LTO the predicate can always be fooled and so we cannot rely on it > as means to prevent ICEs, if the user calls incompatible functions, the > compiler should try to fix it with folding, VCEing or just using zero > constructors in the most bogus of cases. > > And trying to make the predicate more clever can be difficult. When > LTO-building Firefox (without PGO), gimple_check_call_matching_types > returns false 8133 times and those cases can be divided into: > > - 2507x the callee was __builtin_constant_p > - 17x the callee was __builtin_signbit > - 1388x the callee was __builtin_unreachable > > - 4215x would pass the suggested test in comment 5 of the bug. I > examined quite a few and all were exactly the problem discussed in > this PR - they were all deemed incompatible because one parameter > was a reference to a TREE_ADDRESSABLE class. > > - 6x both predicates returned false for a target found by speculative > devirtualization. I tend to think they were both wrong because... > > ...the predicate from comment #5 of the bug is not a good substitute > because it returns false for perfectly fine virtual calls when the type > of the call is a method belonging to an ancestor of the class to which > the actual callee belongs. Thousands of calls to AddRef did not pass > the test. > > Without finding any real case for having the predicate, I decided to > remove its use from everywhere except for check_ic_target because its > comment says: > > /* Perform sanity check on the indirect call target. Due to race conditions, > false function target may be attributed to an indirect call site. If the > call expression type mismatches with the target function's type, expand_call > may ICE. Here we only do very minimal sanity check just to make compiler happy. > Returns true if TARGET is considered ok for call CALL_STMT. */ > > and if some such race conditions really happen and can be detected if > e.g. the number of parameters is clearly off, the compiler should > probably discard the target. But I did not want to keep the original > clumsy predicate and therefore decided to extract the non-problematic > bits of useless_type_conversion_p into: > > /* Check the type of FNDECL and return true if it is likely compatible with the > callee type in CALL. The check is imprecise, the intended use of this > function is that when optimizations like value profiling and speculative > devirtualization somehow guess a clearly wrong target of an indirect call, > they can discard it. */ > > bool > gimple_call_types_likely_match_p (gcall *call, tree fndecl) > { > tree call_type = gimple_call_fntype (call); > tree decl_type = TREE_TYPE (fndecl); > > /* If one is a function and the other a method, that's a mismatch. */ > if (TREE_CODE (call_type) != TREE_CODE (decl_type)) > return false; > /* If the return types are not compatible, bail out. */ > if (!useless_type_conversion_p (TREE_TYPE (call_type), > TREE_TYPE (decl_type))) > return false; > /* If the call was to an unprototyped function, all bets are off. */ > if (!prototype_p (call_type)) > return true; > > /* If the unqualified argument types are compatible, the types match. */ > if (TYPE_ARG_TYPES (call_type) == TYPE_ARG_TYPES (decl_type)) > return true; > > tree call_parm, decl_parm; > for (call_parm = TYPE_ARG_TYPES (call_type), > decl_parm = TYPE_ARG_TYPES (decl_type); > call_parm && decl_parm; > call_parm = TREE_CHAIN (call_parm), > decl_parm = TREE_CHAIN (decl_parm)) > if (!useless_type_conversion_p > (TYPE_MAIN_VARIANT (TREE_VALUE (call_parm)), > TYPE_MAIN_VARIANT (TREE_VALUE (decl_parm)))) > return false; > > /* If there is a mismatch in the number of arguments the functions > are not compatible. */ > if (call_parm || decl_parm) > return false; > > return true; > } > > Crucially, the function is missing the part that does: > > /* Method types should belong to a compatible base class. */ > if (TREE_CODE (inner_type) == METHOD_TYPE > && !useless_type_conversion_p (TYPE_METHOD_BASETYPE (outer_type), > TYPE_METHOD_BASETYPE (inner_type))) > return false; > > Of course, it would be nice if useless_type_conversion_p could detect > ancestors but that does not seem to be easy and doing it IMHO should not > hold us back from fixing this PR now. > > I have also naturally tried to use this predicate in place of the > current gimple_check_call_matching_types and for entire Firefox it > returned false only once - for a call to dlsym because the return type of > > void * <T1690> (void * restrict, const char * restrict) > > was different from: > > void (*<T3a1>) (void) <T16ab> (void *, const char *) > > So I decided that the most common use of > gimple_check_call_matching_types and indeed CIF_MISMATCHED_ARGUMENTS was > not necessary. On the other hand, please note that things like > testsuite/gcc.dg/winline-10.c now get inlined. > > So, what do you think? Is this patch OK? Should I just remove the > checks from check_ic_target and speculative devirtualization too without > introducing the new predicate? Some other concerns? FWIW, this patch > passes bootstrap, LTO profiledbootstrap and testing on x86_64, and I was > able to LTO build and LTO+PGO build Firefox with it and browse BBC News > for a while. I would remove the check from check_ic_target - isn't the race condition avoided now with -fprofile-update=atomic? So - OK with that removed as well. Thanks, Richard. > Thanks, > > Martin > > > > 2019-10-31 Martin Jambor <mjambor@suse.cz> > > PR lto/70929 > * cif-code.def (MISMATCHED_ARGUMENTS): Removed. > * gimple.c (gimple_call_types_likely_match_p): New function. > * gimple.h (gimple_call_types_likely_match_p): Declare. > * cgraph.h (gimple_check_call_matching_types): Remove > * cgraph.c (gimple_check_call_args): Likewise. > (gimple_check_call_matching_types): Likewise. > (symbol_table::create_edge): Do not call > gimple_check_call_matching_types. > (cgraph_edge::make_direct): Likewise. > (cgraph_edge::redirect_call_stmt_to_callee): Call > gimple_call_types_likely_match_p instead, remove trailing > whitespace in the related comment. > * value-prof.c (check_ic_target): Likewise. > * ipa-prop.c (update_indirect_edges_after_inlining): Do not call > gimple_check_call_matching_types. > * ipa-inline.c (early_inliner): Likewise. > > testsuite/ > * g++.dg/lto/pr70929_[01].C: New test. > * gcc.dg/winline-10.c: Adjust for the fact that inlining happens. > --- > gcc/cgraph.c | 127 ++------------------------- > gcc/cgraph.h | 2 - > gcc/cif-code.def | 4 - > gcc/gimple.c | 46 ++++++++++ > gcc/gimple.h | 1 + > gcc/ipa-inline.c | 8 -- > gcc/ipa-prop.c | 5 -- > gcc/testsuite/g++.dg/lto/pr70929_0.C | 18 ++++ > gcc/testsuite/g++.dg/lto/pr70929_1.C | 10 +++ > gcc/testsuite/gcc.dg/winline-10.c | 6 +- > gcc/value-prof.c | 11 ++- > 11 files changed, 89 insertions(+), 149 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/lto/pr70929_0.C > create mode 100644 gcc/testsuite/g++.dg/lto/pr70929_1.C > > diff --git a/gcc/cgraph.c b/gcc/cgraph.c > index 8057ccdb7c0..95acf36936f 100644 > --- a/gcc/cgraph.c > +++ b/gcc/cgraph.c > @@ -877,19 +877,8 @@ symbol_table::create_edge (cgraph_node *caller, cgraph_node *callee, > edge->can_throw_external > = call_stmt ? stmt_can_throw_external (DECL_STRUCT_FUNCTION (caller->decl), > call_stmt) : false; > - if (call_stmt > - && callee && callee->decl > - && !gimple_check_call_matching_types (call_stmt, callee->decl, > - false)) > - { > - edge->inline_failed = CIF_MISMATCHED_ARGUMENTS; > - edge->call_stmt_cannot_inline_p = true; > - } > - else > - { > - edge->inline_failed = CIF_FUNCTION_NOT_CONSIDERED; > - edge->call_stmt_cannot_inline_p = false; > - } > + edge->inline_failed = CIF_FUNCTION_NOT_CONSIDERED; > + edge->call_stmt_cannot_inline_p = false; > > if (opt_for_fn (edge->caller->decl, flag_devirtualize) > && call_stmt && DECL_STRUCT_FUNCTION (caller->decl)) > @@ -1251,13 +1240,6 @@ cgraph_edge::make_direct (cgraph_node *callee) > /* Insert to callers list of the new callee. */ > edge->set_callee (callee); > > - if (call_stmt > - && !gimple_check_call_matching_types (call_stmt, callee->decl, false)) > - { > - call_stmt_cannot_inline_p = true; > - inline_failed = CIF_MISMATCHED_ARGUMENTS; > - } > - > /* We need to re-determine the inlining status of the edge. */ > initialize_inline_failed (edge); > return edge; > @@ -1286,13 +1268,12 @@ cgraph_edge::redirect_call_stmt_to_callee (void) > substitution), forget about speculating. */ > if (decl) > e = e->resolve_speculation (decl); > - /* If types do not match, speculation was likely wrong. > + /* If types do not match, speculation was likely wrong. > The direct edge was possibly redirected to the clone with a different > - signature. We did not update the call statement yet, so compare it > + signature. We did not update the call statement yet, so compare it > with the reference that still points to the proper type. */ > - else if (!gimple_check_call_matching_types (e->call_stmt, > - ref->referred->decl, > - true)) > + else if (!gimple_call_types_likely_match_p (e->call_stmt, > + ref->referred->decl)) > { > if (dump_file) > fprintf (dump_file, "Not expanding speculative call of %s -> %s\n" > @@ -3629,102 +3610,6 @@ cgraph_node::get_fun () const > return fun; > } > > -/* Verify if the type of the argument matches that of the function > - declaration. If we cannot verify this or there is a mismatch, > - return false. */ > - > -static bool > -gimple_check_call_args (gimple *stmt, tree fndecl, bool args_count_match) > -{ > - tree parms, p; > - unsigned int i, nargs; > - > - /* Calls to internal functions always match their signature. */ > - if (gimple_call_internal_p (stmt)) > - return true; > - > - nargs = gimple_call_num_args (stmt); > - > - /* Get argument types for verification. */ > - if (fndecl) > - parms = TYPE_ARG_TYPES (TREE_TYPE (fndecl)); > - else > - parms = TYPE_ARG_TYPES (gimple_call_fntype (stmt)); > - > - /* Verify if the type of the argument matches that of the function > - declaration. If we cannot verify this or there is a mismatch, > - return false. */ > - if (fndecl && DECL_ARGUMENTS (fndecl)) > - { > - for (i = 0, p = DECL_ARGUMENTS (fndecl); > - i < nargs; > - i++, p = DECL_CHAIN (p)) > - { > - tree arg; > - /* We cannot distinguish a varargs function from the case > - of excess parameters, still deferring the inlining decision > - to the callee is possible. */ > - if (!p) > - break; > - arg = gimple_call_arg (stmt, i); > - if (p == error_mark_node > - || DECL_ARG_TYPE (p) == error_mark_node > - || arg == error_mark_node > - || (!types_compatible_p (DECL_ARG_TYPE (p), TREE_TYPE (arg)) > - && !fold_convertible_p (DECL_ARG_TYPE (p), arg))) > - return false; > - } > - if (args_count_match && p) > - return false; > - } > - else if (parms) > - { > - for (i = 0, p = parms; i < nargs; i++, p = TREE_CHAIN (p)) > - { > - tree arg; > - /* If this is a varargs function defer inlining decision > - to callee. */ > - if (!p) > - break; > - arg = gimple_call_arg (stmt, i); > - if (TREE_VALUE (p) == error_mark_node > - || arg == error_mark_node > - || TREE_CODE (TREE_VALUE (p)) == VOID_TYPE > - || (!types_compatible_p (TREE_VALUE (p), TREE_TYPE (arg)) > - && !fold_convertible_p (TREE_VALUE (p), arg))) > - return false; > - } > - } > - else > - { > - if (nargs != 0) > - return false; > - } > - return true; > -} > - > -/* Verify if the type of the argument and lhs of CALL_STMT matches > - that of the function declaration CALLEE. If ARGS_COUNT_MATCH is > - true, the arg count needs to be the same. > - If we cannot verify this or there is a mismatch, return false. */ > - > -bool > -gimple_check_call_matching_types (gimple *call_stmt, tree callee, > - bool args_count_match) > -{ > - tree lhs; > - > - if ((DECL_RESULT (callee) > - && !DECL_BY_REFERENCE (DECL_RESULT (callee)) > - && (lhs = gimple_call_lhs (call_stmt)) != NULL_TREE > - && !useless_type_conversion_p (TREE_TYPE (DECL_RESULT (callee)), > - TREE_TYPE (lhs)) > - && !fold_convertible_p (TREE_TYPE (DECL_RESULT (callee)), lhs)) > - || !gimple_check_call_args (call_stmt, callee, args_count_match)) > - return false; > - return true; > -} > - > /* Reset all state within cgraph.c so that we can rerun the compiler > within the same process. For use by toplev::finalize. */ > > diff --git a/gcc/cgraph.h b/gcc/cgraph.h > index a7f357f0b5c..990fba10708 100644 > --- a/gcc/cgraph.h > +++ b/gcc/cgraph.h > @@ -2462,8 +2462,6 @@ bool cgraph_function_possibly_inlined_p (tree); > const char* cgraph_inline_failed_string (cgraph_inline_failed_t); > cgraph_inline_failed_type_t cgraph_inline_failed_type (cgraph_inline_failed_t); > > -extern bool gimple_check_call_matching_types (gimple *, tree, bool); > - > /* In cgraphunit.c */ > void cgraphunit_c_finalize (void); > > diff --git a/gcc/cif-code.def b/gcc/cif-code.def > index 8676ee1c0f3..a154f24f13d 100644 > --- a/gcc/cif-code.def > +++ b/gcc/cif-code.def > @@ -95,10 +95,6 @@ DEFCIFCODE(NEVER_CALL, CIF_FINAL_NORMAL, > DEFCIFCODE(NOT_DECLARED_INLINED, CIF_FINAL_NORMAL, > N_("function not declared inline and code size would grow")) > > -/* Caller and callee disagree on the arguments. */ > -DEFCIFCODE(MISMATCHED_ARGUMENTS, CIF_FINAL_ERROR, > - N_("mismatched arguments")) > - > /* Caller and callee disagree on the arguments. */ > DEFCIFCODE(LTO_MISMATCHED_DECLARATIONS, CIF_FINAL_ERROR, > N_("mismatched declarations during linktime optimization")) > diff --git a/gcc/gimple.c b/gcc/gimple.c > index a874c29454c..4d6f48b33a3 100644 > --- a/gcc/gimple.c > +++ b/gcc/gimple.c > @@ -2802,6 +2802,52 @@ gimple_call_combined_fn (const gimple *stmt) > return CFN_LAST; > } > > +/* Check the type of FNDECL and return true if it is likely compatible with the > + callee type in CALL. The check is imprecise, the intended use of this > + function is that when optimizations like value profiling and speculative > + devirtualization somehow guess a clearly wrong target of an indirect call, > + they can discard it. */ > + > +bool > +gimple_call_types_likely_match_p (gcall *call, tree fndecl) > +{ > + tree call_type = gimple_call_fntype (call); > + tree decl_type = TREE_TYPE (fndecl); > + > + /* If one is a function and the other a method, that's a mismatch. */ > + if (TREE_CODE (call_type) != TREE_CODE (decl_type)) > + return false; > + /* If the return types are not compatible, bail out. */ > + if (!useless_type_conversion_p (TREE_TYPE (call_type), > + TREE_TYPE (decl_type))) > + return false; > + /* If the call was to an unprototyped function, all bets are off. */ > + if (!prototype_p (call_type)) > + return true; > + > + /* If the unqualified argument types are compatible, the types match. */ > + if (TYPE_ARG_TYPES (call_type) == TYPE_ARG_TYPES (decl_type)) > + return true; > + > + tree call_parm, decl_parm; > + for (call_parm = TYPE_ARG_TYPES (call_type), > + decl_parm = TYPE_ARG_TYPES (decl_type); > + call_parm && decl_parm; > + call_parm = TREE_CHAIN (call_parm), > + decl_parm = TREE_CHAIN (decl_parm)) > + if (!useless_type_conversion_p > + (TYPE_MAIN_VARIANT (TREE_VALUE (call_parm)), > + TYPE_MAIN_VARIANT (TREE_VALUE (decl_parm)))) > + return false; > + > + /* If there is a mismatch in the number of arguments the functions > + are not compatible. */ > + if (call_parm || decl_parm) > + return false; > + > + return true; > +} > + > /* Return true if STMT clobbers memory. STMT is required to be a > GIMPLE_ASM. */ > > diff --git a/gcc/gimple.h b/gcc/gimple.h > index cf1f8da5ae2..853ecc0d5b9 100644 > --- a/gcc/gimple.h > +++ b/gcc/gimple.h > @@ -1550,6 +1550,7 @@ extern alias_set_type gimple_get_alias_set (tree); > extern bool gimple_ior_addresses_taken (bitmap, gimple *); > extern bool gimple_builtin_call_types_compatible_p (const gimple *, tree); > extern combined_fn gimple_call_combined_fn (const gimple *); > +extern bool gimple_call_types_likely_match_p (gcall *, tree); > extern bool gimple_call_operator_delete_p (const gcall *); > extern bool gimple_call_builtin_p (const gimple *); > extern bool gimple_call_builtin_p (const gimple *, enum built_in_class); > diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c > index a7ef7faa3a0..4bc6faeb9f9 100644 > --- a/gcc/ipa-inline.c > +++ b/gcc/ipa-inline.c > @@ -2912,14 +2912,6 @@ early_inliner (function *fun) > = estimate_num_insns (edge->call_stmt, &eni_size_weights); > es->call_stmt_time > = estimate_num_insns (edge->call_stmt, &eni_time_weights); > - > - if (edge->callee->decl > - && !gimple_check_call_matching_types ( > - edge->call_stmt, edge->callee->decl, false)) > - { > - edge->inline_failed = CIF_MISMATCHED_ARGUMENTS; > - edge->call_stmt_cannot_inline_p = true; > - } > } > if (iterations < PARAM_VALUE (PARAM_EARLY_INLINER_MAX_ITERATIONS) - 1) > ipa_update_overall_fn_summary (node); > diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c > index 10fe1bc929f..2ccd2f59139 100644 > --- a/gcc/ipa-prop.c > +++ b/gcc/ipa-prop.c > @@ -3476,11 +3476,6 @@ update_indirect_edges_after_inlining (struct cgraph_edge *cs, > else if (new_direct_edge) > { > new_direct_edge->indirect_inlining_edge = 1; > - if (new_direct_edge->call_stmt) > - new_direct_edge->call_stmt_cannot_inline_p > - = !gimple_check_call_matching_types ( > - new_direct_edge->call_stmt, > - new_direct_edge->callee->decl, false); > if (new_edges) > { > new_edges->safe_push (new_direct_edge); > diff --git a/gcc/testsuite/g++.dg/lto/pr70929_0.C b/gcc/testsuite/g++.dg/lto/pr70929_0.C > new file mode 100644 > index 00000000000..c96fb1c743a > --- /dev/null > +++ b/gcc/testsuite/g++.dg/lto/pr70929_0.C > @@ -0,0 +1,18 @@ > +// { dg-lto-do run } > +// { dg-lto-options { "-O3 -flto" } } > + > +struct s > +{ > + int a; > + s() {a=1;} > + ~s() {} > +}; > +int t(struct s s); > +int main() > +{ > + s s; > + int v=t(s); > + if (!__builtin_constant_p (v)) > + __builtin_abort (); > + return 0; > +} > diff --git a/gcc/testsuite/g++.dg/lto/pr70929_1.C b/gcc/testsuite/g++.dg/lto/pr70929_1.C > new file mode 100644 > index 00000000000..b33aa8f35f0 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/lto/pr70929_1.C > @@ -0,0 +1,10 @@ > +struct s > +{ > + int a; > + s() {a=1;} > + ~s() {} > +}; > +int t(struct s s) > +{ > + return s.a; > +} > diff --git a/gcc/testsuite/gcc.dg/winline-10.c b/gcc/testsuite/gcc.dg/winline-10.c > index dfc868fa85a..2df5addaebc 100644 > --- a/gcc/testsuite/gcc.dg/winline-10.c > +++ b/gcc/testsuite/gcc.dg/winline-10.c > @@ -1,9 +1,9 @@ > /* { dg-do compile } */ > -/* { dg-options "-O2 -Winline" } */ > +/* { dg-options "-O2 -Winline -fopt-info-optimized-inline=stderr" } */ > > struct s { int a; }; > > -inline void f (x) /* { dg-warning "inlining .* mismatched arg" } */ > +inline void f (x) > int x; > { > asm (""); > @@ -11,7 +11,7 @@ inline void f (x) /* { dg-warning "inlining .* mismatched arg" } */ > > void g (struct s x) > { > - f (x); /* { dg-message "called from here" } */ > + f (x); /* { dg-optimized "Inlining f.* into g" } */ > } > > void f (int x); /* { dg-warning "follows non-prototype definition" } */ > diff --git a/gcc/value-prof.c b/gcc/value-prof.c > index 55ea0973a03..797051b57be 100644 > --- a/gcc/value-prof.c > +++ b/gcc/value-prof.c > @@ -1245,16 +1245,15 @@ find_func_by_profile_id (int profile_id) > return NULL; > } > > -/* Perform sanity check on the indirect call target. Due to race conditions, > - false function target may be attributed to an indirect call site. If the > - call expression type mismatches with the target function's type, expand_call > - may ICE. Here we only do very minimal sanity check just to make compiler happy. > - Returns true if TARGET is considered ok for call CALL_STMT. */ > +/* Perform sanity check on the indirect call target. Due to race conditions, > + false function target may be attributed to an indirect call site. If the > + call expression type mismatches with the target function's type assume that > + is the case. Returns true if TARGET is considered ok for call CALL_STMT. */ > > bool > check_ic_target (gcall *call_stmt, struct cgraph_node *target) > { > - if (gimple_check_call_matching_types (call_stmt, target->decl, true)) > + if (gimple_call_types_likely_match_p (call_stmt, target->decl)) > return true; > > if (dump_enabled_p ()) >
diff --git a/gcc/cgraph.c b/gcc/cgraph.c index 8057ccdb7c0..95acf36936f 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -877,19 +877,8 @@ symbol_table::create_edge (cgraph_node *caller, cgraph_node *callee, edge->can_throw_external = call_stmt ? stmt_can_throw_external (DECL_STRUCT_FUNCTION (caller->decl), call_stmt) : false; - if (call_stmt - && callee && callee->decl - && !gimple_check_call_matching_types (call_stmt, callee->decl, - false)) - { - edge->inline_failed = CIF_MISMATCHED_ARGUMENTS; - edge->call_stmt_cannot_inline_p = true; - } - else - { - edge->inline_failed = CIF_FUNCTION_NOT_CONSIDERED; - edge->call_stmt_cannot_inline_p = false; - } + edge->inline_failed = CIF_FUNCTION_NOT_CONSIDERED; + edge->call_stmt_cannot_inline_p = false; if (opt_for_fn (edge->caller->decl, flag_devirtualize) && call_stmt && DECL_STRUCT_FUNCTION (caller->decl)) @@ -1251,13 +1240,6 @@ cgraph_edge::make_direct (cgraph_node *callee) /* Insert to callers list of the new callee. */ edge->set_callee (callee); - if (call_stmt - && !gimple_check_call_matching_types (call_stmt, callee->decl, false)) - { - call_stmt_cannot_inline_p = true; - inline_failed = CIF_MISMATCHED_ARGUMENTS; - } - /* We need to re-determine the inlining status of the edge. */ initialize_inline_failed (edge); return edge; @@ -1286,13 +1268,12 @@ cgraph_edge::redirect_call_stmt_to_callee (void) substitution), forget about speculating. */ if (decl) e = e->resolve_speculation (decl); - /* If types do not match, speculation was likely wrong. + /* If types do not match, speculation was likely wrong. The direct edge was possibly redirected to the clone with a different - signature. We did not update the call statement yet, so compare it + signature. We did not update the call statement yet, so compare it with the reference that still points to the proper type. */ - else if (!gimple_check_call_matching_types (e->call_stmt, - ref->referred->decl, - true)) + else if (!gimple_call_types_likely_match_p (e->call_stmt, + ref->referred->decl)) { if (dump_file) fprintf (dump_file, "Not expanding speculative call of %s -> %s\n" @@ -3629,102 +3610,6 @@ cgraph_node::get_fun () const return fun; } -/* Verify if the type of the argument matches that of the function - declaration. If we cannot verify this or there is a mismatch, - return false. */ - -static bool -gimple_check_call_args (gimple *stmt, tree fndecl, bool args_count_match) -{ - tree parms, p; - unsigned int i, nargs; - - /* Calls to internal functions always match their signature. */ - if (gimple_call_internal_p (stmt)) - return true; - - nargs = gimple_call_num_args (stmt); - - /* Get argument types for verification. */ - if (fndecl) - parms = TYPE_ARG_TYPES (TREE_TYPE (fndecl)); - else - parms = TYPE_ARG_TYPES (gimple_call_fntype (stmt)); - - /* Verify if the type of the argument matches that of the function - declaration. If we cannot verify this or there is a mismatch, - return false. */ - if (fndecl && DECL_ARGUMENTS (fndecl)) - { - for (i = 0, p = DECL_ARGUMENTS (fndecl); - i < nargs; - i++, p = DECL_CHAIN (p)) - { - tree arg; - /* We cannot distinguish a varargs function from the case - of excess parameters, still deferring the inlining decision - to the callee is possible. */ - if (!p) - break; - arg = gimple_call_arg (stmt, i); - if (p == error_mark_node - || DECL_ARG_TYPE (p) == error_mark_node - || arg == error_mark_node - || (!types_compatible_p (DECL_ARG_TYPE (p), TREE_TYPE (arg)) - && !fold_convertible_p (DECL_ARG_TYPE (p), arg))) - return false; - } - if (args_count_match && p) - return false; - } - else if (parms) - { - for (i = 0, p = parms; i < nargs; i++, p = TREE_CHAIN (p)) - { - tree arg; - /* If this is a varargs function defer inlining decision - to callee. */ - if (!p) - break; - arg = gimple_call_arg (stmt, i); - if (TREE_VALUE (p) == error_mark_node - || arg == error_mark_node - || TREE_CODE (TREE_VALUE (p)) == VOID_TYPE - || (!types_compatible_p (TREE_VALUE (p), TREE_TYPE (arg)) - && !fold_convertible_p (TREE_VALUE (p), arg))) - return false; - } - } - else - { - if (nargs != 0) - return false; - } - return true; -} - -/* Verify if the type of the argument and lhs of CALL_STMT matches - that of the function declaration CALLEE. If ARGS_COUNT_MATCH is - true, the arg count needs to be the same. - If we cannot verify this or there is a mismatch, return false. */ - -bool -gimple_check_call_matching_types (gimple *call_stmt, tree callee, - bool args_count_match) -{ - tree lhs; - - if ((DECL_RESULT (callee) - && !DECL_BY_REFERENCE (DECL_RESULT (callee)) - && (lhs = gimple_call_lhs (call_stmt)) != NULL_TREE - && !useless_type_conversion_p (TREE_TYPE (DECL_RESULT (callee)), - TREE_TYPE (lhs)) - && !fold_convertible_p (TREE_TYPE (DECL_RESULT (callee)), lhs)) - || !gimple_check_call_args (call_stmt, callee, args_count_match)) - return false; - return true; -} - /* Reset all state within cgraph.c so that we can rerun the compiler within the same process. For use by toplev::finalize. */ diff --git a/gcc/cgraph.h b/gcc/cgraph.h index a7f357f0b5c..990fba10708 100644 --- a/gcc/cgraph.h +++ b/gcc/cgraph.h @@ -2462,8 +2462,6 @@ bool cgraph_function_possibly_inlined_p (tree); const char* cgraph_inline_failed_string (cgraph_inline_failed_t); cgraph_inline_failed_type_t cgraph_inline_failed_type (cgraph_inline_failed_t); -extern bool gimple_check_call_matching_types (gimple *, tree, bool); - /* In cgraphunit.c */ void cgraphunit_c_finalize (void); diff --git a/gcc/cif-code.def b/gcc/cif-code.def index 8676ee1c0f3..a154f24f13d 100644 --- a/gcc/cif-code.def +++ b/gcc/cif-code.def @@ -95,10 +95,6 @@ DEFCIFCODE(NEVER_CALL, CIF_FINAL_NORMAL, DEFCIFCODE(NOT_DECLARED_INLINED, CIF_FINAL_NORMAL, N_("function not declared inline and code size would grow")) -/* Caller and callee disagree on the arguments. */ -DEFCIFCODE(MISMATCHED_ARGUMENTS, CIF_FINAL_ERROR, - N_("mismatched arguments")) - /* Caller and callee disagree on the arguments. */ DEFCIFCODE(LTO_MISMATCHED_DECLARATIONS, CIF_FINAL_ERROR, N_("mismatched declarations during linktime optimization")) diff --git a/gcc/gimple.c b/gcc/gimple.c index a874c29454c..4d6f48b33a3 100644 --- a/gcc/gimple.c +++ b/gcc/gimple.c @@ -2802,6 +2802,52 @@ gimple_call_combined_fn (const gimple *stmt) return CFN_LAST; } +/* Check the type of FNDECL and return true if it is likely compatible with the + callee type in CALL. The check is imprecise, the intended use of this + function is that when optimizations like value profiling and speculative + devirtualization somehow guess a clearly wrong target of an indirect call, + they can discard it. */ + +bool +gimple_call_types_likely_match_p (gcall *call, tree fndecl) +{ + tree call_type = gimple_call_fntype (call); + tree decl_type = TREE_TYPE (fndecl); + + /* If one is a function and the other a method, that's a mismatch. */ + if (TREE_CODE (call_type) != TREE_CODE (decl_type)) + return false; + /* If the return types are not compatible, bail out. */ + if (!useless_type_conversion_p (TREE_TYPE (call_type), + TREE_TYPE (decl_type))) + return false; + /* If the call was to an unprototyped function, all bets are off. */ + if (!prototype_p (call_type)) + return true; + + /* If the unqualified argument types are compatible, the types match. */ + if (TYPE_ARG_TYPES (call_type) == TYPE_ARG_TYPES (decl_type)) + return true; + + tree call_parm, decl_parm; + for (call_parm = TYPE_ARG_TYPES (call_type), + decl_parm = TYPE_ARG_TYPES (decl_type); + call_parm && decl_parm; + call_parm = TREE_CHAIN (call_parm), + decl_parm = TREE_CHAIN (decl_parm)) + if (!useless_type_conversion_p + (TYPE_MAIN_VARIANT (TREE_VALUE (call_parm)), + TYPE_MAIN_VARIANT (TREE_VALUE (decl_parm)))) + return false; + + /* If there is a mismatch in the number of arguments the functions + are not compatible. */ + if (call_parm || decl_parm) + return false; + + return true; +} + /* Return true if STMT clobbers memory. STMT is required to be a GIMPLE_ASM. */ diff --git a/gcc/gimple.h b/gcc/gimple.h index cf1f8da5ae2..853ecc0d5b9 100644 --- a/gcc/gimple.h +++ b/gcc/gimple.h @@ -1550,6 +1550,7 @@ extern alias_set_type gimple_get_alias_set (tree); extern bool gimple_ior_addresses_taken (bitmap, gimple *); extern bool gimple_builtin_call_types_compatible_p (const gimple *, tree); extern combined_fn gimple_call_combined_fn (const gimple *); +extern bool gimple_call_types_likely_match_p (gcall *, tree); extern bool gimple_call_operator_delete_p (const gcall *); extern bool gimple_call_builtin_p (const gimple *); extern bool gimple_call_builtin_p (const gimple *, enum built_in_class); diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c index a7ef7faa3a0..4bc6faeb9f9 100644 --- a/gcc/ipa-inline.c +++ b/gcc/ipa-inline.c @@ -2912,14 +2912,6 @@ early_inliner (function *fun) = estimate_num_insns (edge->call_stmt, &eni_size_weights); es->call_stmt_time = estimate_num_insns (edge->call_stmt, &eni_time_weights); - - if (edge->callee->decl - && !gimple_check_call_matching_types ( - edge->call_stmt, edge->callee->decl, false)) - { - edge->inline_failed = CIF_MISMATCHED_ARGUMENTS; - edge->call_stmt_cannot_inline_p = true; - } } if (iterations < PARAM_VALUE (PARAM_EARLY_INLINER_MAX_ITERATIONS) - 1) ipa_update_overall_fn_summary (node); diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c index 10fe1bc929f..2ccd2f59139 100644 --- a/gcc/ipa-prop.c +++ b/gcc/ipa-prop.c @@ -3476,11 +3476,6 @@ update_indirect_edges_after_inlining (struct cgraph_edge *cs, else if (new_direct_edge) { new_direct_edge->indirect_inlining_edge = 1; - if (new_direct_edge->call_stmt) - new_direct_edge->call_stmt_cannot_inline_p - = !gimple_check_call_matching_types ( - new_direct_edge->call_stmt, - new_direct_edge->callee->decl, false); if (new_edges) { new_edges->safe_push (new_direct_edge); diff --git a/gcc/testsuite/g++.dg/lto/pr70929_0.C b/gcc/testsuite/g++.dg/lto/pr70929_0.C new file mode 100644 index 00000000000..c96fb1c743a --- /dev/null +++ b/gcc/testsuite/g++.dg/lto/pr70929_0.C @@ -0,0 +1,18 @@ +// { dg-lto-do run } +// { dg-lto-options { "-O3 -flto" } } + +struct s +{ + int a; + s() {a=1;} + ~s() {} +}; +int t(struct s s); +int main() +{ + s s; + int v=t(s); + if (!__builtin_constant_p (v)) + __builtin_abort (); + return 0; +} diff --git a/gcc/testsuite/g++.dg/lto/pr70929_1.C b/gcc/testsuite/g++.dg/lto/pr70929_1.C new file mode 100644 index 00000000000..b33aa8f35f0 --- /dev/null +++ b/gcc/testsuite/g++.dg/lto/pr70929_1.C @@ -0,0 +1,10 @@ +struct s +{ + int a; + s() {a=1;} + ~s() {} +}; +int t(struct s s) +{ + return s.a; +} diff --git a/gcc/testsuite/gcc.dg/winline-10.c b/gcc/testsuite/gcc.dg/winline-10.c index dfc868fa85a..2df5addaebc 100644 --- a/gcc/testsuite/gcc.dg/winline-10.c +++ b/gcc/testsuite/gcc.dg/winline-10.c @@ -1,9 +1,9 @@ /* { dg-do compile } */ -/* { dg-options "-O2 -Winline" } */ +/* { dg-options "-O2 -Winline -fopt-info-optimized-inline=stderr" } */ struct s { int a; }; -inline void f (x) /* { dg-warning "inlining .* mismatched arg" } */ +inline void f (x) int x; { asm (""); @@ -11,7 +11,7 @@ inline void f (x) /* { dg-warning "inlining .* mismatched arg" } */ void g (struct s x) { - f (x); /* { dg-message "called from here" } */ + f (x); /* { dg-optimized "Inlining f.* into g" } */ } void f (int x); /* { dg-warning "follows non-prototype definition" } */ diff --git a/gcc/value-prof.c b/gcc/value-prof.c index 55ea0973a03..797051b57be 100644 --- a/gcc/value-prof.c +++ b/gcc/value-prof.c @@ -1245,16 +1245,15 @@ find_func_by_profile_id (int profile_id) return NULL; } -/* Perform sanity check on the indirect call target. Due to race conditions, - false function target may be attributed to an indirect call site. If the - call expression type mismatches with the target function's type, expand_call - may ICE. Here we only do very minimal sanity check just to make compiler happy. - Returns true if TARGET is considered ok for call CALL_STMT. */ +/* Perform sanity check on the indirect call target. Due to race conditions, + false function target may be attributed to an indirect call site. If the + call expression type mismatches with the target function's type assume that + is the case. Returns true if TARGET is considered ok for call CALL_STMT. */ bool check_ic_target (gcall *call_stmt, struct cgraph_node *target) { - if (gimple_check_call_matching_types (call_stmt, target->decl, true)) + if (gimple_call_types_likely_match_p (call_stmt, target->decl)) return true; if (dump_enabled_p ())
Hi, I have spent some more time looking into PR 70929, how gimple_check_call_matching_types behaves when LTO-building Firefox to see what could replace it or if we perhaps could remove it. TL;DR: I believe it can and should be removed, possibly except the use in value-prof.c where I replaced it with a clearly imprecise predicate to catch cases where the profile info is corrupted and since I had it, I also ended up using it for speculative devirtualization, in case it got its guess somehow wrong (but I have not seen either of the two cases happening). See the patch below. More details: With LTO the predicate can always be fooled and so we cannot rely on it as means to prevent ICEs, if the user calls incompatible functions, the compiler should try to fix it with folding, VCEing or just using zero constructors in the most bogus of cases. And trying to make the predicate more clever can be difficult. When LTO-building Firefox (without PGO), gimple_check_call_matching_types returns false 8133 times and those cases can be divided into: - 2507x the callee was __builtin_constant_p - 17x the callee was __builtin_signbit - 1388x the callee was __builtin_unreachable - 4215x would pass the suggested test in comment 5 of the bug. I examined quite a few and all were exactly the problem discussed in this PR - they were all deemed incompatible because one parameter was a reference to a TREE_ADDRESSABLE class. - 6x both predicates returned false for a target found by speculative devirtualization. I tend to think they were both wrong because... ...the predicate from comment #5 of the bug is not a good substitute because it returns false for perfectly fine virtual calls when the type of the call is a method belonging to an ancestor of the class to which the actual callee belongs. Thousands of calls to AddRef did not pass the test. Without finding any real case for having the predicate, I decided to remove its use from everywhere except for check_ic_target because its comment says: /* Perform sanity check on the indirect call target. Due to race conditions, false function target may be attributed to an indirect call site. If the call expression type mismatches with the target function's type, expand_call may ICE. Here we only do very minimal sanity check just to make compiler happy. Returns true if TARGET is considered ok for call CALL_STMT. */ and if some such race conditions really happen and can be detected if e.g. the number of parameters is clearly off, the compiler should probably discard the target. But I did not want to keep the original clumsy predicate and therefore decided to extract the non-problematic bits of useless_type_conversion_p into: /* Check the type of FNDECL and return true if it is likely compatible with the callee type in CALL. The check is imprecise, the intended use of this function is that when optimizations like value profiling and speculative devirtualization somehow guess a clearly wrong target of an indirect call, they can discard it. */ bool gimple_call_types_likely_match_p (gcall *call, tree fndecl) { tree call_type = gimple_call_fntype (call); tree decl_type = TREE_TYPE (fndecl); /* If one is a function and the other a method, that's a mismatch. */ if (TREE_CODE (call_type) != TREE_CODE (decl_type)) return false; /* If the return types are not compatible, bail out. */ if (!useless_type_conversion_p (TREE_TYPE (call_type), TREE_TYPE (decl_type))) return false; /* If the call was to an unprototyped function, all bets are off. */ if (!prototype_p (call_type)) return true; /* If the unqualified argument types are compatible, the types match. */ if (TYPE_ARG_TYPES (call_type) == TYPE_ARG_TYPES (decl_type)) return true; tree call_parm, decl_parm; for (call_parm = TYPE_ARG_TYPES (call_type), decl_parm = TYPE_ARG_TYPES (decl_type); call_parm && decl_parm; call_parm = TREE_CHAIN (call_parm), decl_parm = TREE_CHAIN (decl_parm)) if (!useless_type_conversion_p (TYPE_MAIN_VARIANT (TREE_VALUE (call_parm)), TYPE_MAIN_VARIANT (TREE_VALUE (decl_parm)))) return false; /* If there is a mismatch in the number of arguments the functions are not compatible. */ if (call_parm || decl_parm) return false; return true; } Crucially, the function is missing the part that does: /* Method types should belong to a compatible base class. */ if (TREE_CODE (inner_type) == METHOD_TYPE && !useless_type_conversion_p (TYPE_METHOD_BASETYPE (outer_type), TYPE_METHOD_BASETYPE (inner_type))) return false; Of course, it would be nice if useless_type_conversion_p could detect ancestors but that does not seem to be easy and doing it IMHO should not hold us back from fixing this PR now. I have also naturally tried to use this predicate in place of the current gimple_check_call_matching_types and for entire Firefox it returned false only once - for a call to dlsym because the return type of void * <T1690> (void * restrict, const char * restrict) was different from: void (*<T3a1>) (void) <T16ab> (void *, const char *) So I decided that the most common use of gimple_check_call_matching_types and indeed CIF_MISMATCHED_ARGUMENTS was not necessary. On the other hand, please note that things like testsuite/gcc.dg/winline-10.c now get inlined. So, what do you think? Is this patch OK? Should I just remove the checks from check_ic_target and speculative devirtualization too without introducing the new predicate? Some other concerns? FWIW, this patch passes bootstrap, LTO profiledbootstrap and testing on x86_64, and I was able to LTO build and LTO+PGO build Firefox with it and browse BBC News for a while. Thanks, Martin 2019-10-31 Martin Jambor <mjambor@suse.cz> PR lto/70929 * cif-code.def (MISMATCHED_ARGUMENTS): Removed. * gimple.c (gimple_call_types_likely_match_p): New function. * gimple.h (gimple_call_types_likely_match_p): Declare. * cgraph.h (gimple_check_call_matching_types): Remove * cgraph.c (gimple_check_call_args): Likewise. (gimple_check_call_matching_types): Likewise. (symbol_table::create_edge): Do not call gimple_check_call_matching_types. (cgraph_edge::make_direct): Likewise. (cgraph_edge::redirect_call_stmt_to_callee): Call gimple_call_types_likely_match_p instead, remove trailing whitespace in the related comment. * value-prof.c (check_ic_target): Likewise. * ipa-prop.c (update_indirect_edges_after_inlining): Do not call gimple_check_call_matching_types. * ipa-inline.c (early_inliner): Likewise. testsuite/ * g++.dg/lto/pr70929_[01].C: New test. * gcc.dg/winline-10.c: Adjust for the fact that inlining happens. --- gcc/cgraph.c | 127 ++------------------------- gcc/cgraph.h | 2 - gcc/cif-code.def | 4 - gcc/gimple.c | 46 ++++++++++ gcc/gimple.h | 1 + gcc/ipa-inline.c | 8 -- gcc/ipa-prop.c | 5 -- gcc/testsuite/g++.dg/lto/pr70929_0.C | 18 ++++ gcc/testsuite/g++.dg/lto/pr70929_1.C | 10 +++ gcc/testsuite/gcc.dg/winline-10.c | 6 +- gcc/value-prof.c | 11 ++- 11 files changed, 89 insertions(+), 149 deletions(-) create mode 100644 gcc/testsuite/g++.dg/lto/pr70929_0.C create mode 100644 gcc/testsuite/g++.dg/lto/pr70929_1.C