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 <mjam...@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 ())
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
