Hi
On Mon, Nov 04 2019, Richard Biener wrote:
> 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.
>
OK, this is what I committed after a bootstrap, LTO bootstrap and LTO
profiledbootstrap and testing on x86_64 and testing. I did not try
Firefox now because that does not build with trunk because of PR 92206.
Thanks,
Martin
2019-11-07 Martin Jambor <mjam...@suse.cz>
PR lto/70929
* cif-code.def (MISMATCHED_ARGUMENTS): Removed.
* 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): Likewise.
* value-prof.h (check_ic_target): Remove.
* value-prof.c (check_ic_target): Remove.
(gimple_ic_transform): Do nat call check_ic_target.
* auto-profile.c (function_instance::find_icall_target_map): Likewise.
(afdo_indirect_call): 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/auto-profile.c | 4 +-
gcc/cgraph.c | 139 +--------------------------
gcc/cgraph.h | 2 -
gcc/cif-code.def | 4 -
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 | 30 ------
10 files changed, 35 insertions(+), 191 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/auto-profile.c b/gcc/auto-profile.c
index ee1a83abce2..4d8fb525cb1 100644
--- a/gcc/auto-profile.c
+++ b/gcc/auto-profile.c
@@ -606,8 +606,6 @@ function_instance::find_icall_target_map (gcall *stmt,
get_identifier (afdo_string_table->get_name (callee)));
if (node == NULL)
continue;
- if (!check_ic_target (stmt, node))
- continue;
(*map)[callee] = iter->second->total_count ();
ret += iter->second->total_count ();
}
@@ -1034,7 +1032,7 @@ afdo_indirect_call (gimple_stmt_iterator *gsi, const
icall_target_map &map,
print_generic_expr (dump_file, direct_call->decl, TDF_SLIM);
}
- if (direct_call == NULL || !check_ic_target (stmt, direct_call))
+ if (direct_call == NULL)
{
if (dump_file)
fprintf (dump_file, " not transforming\n");
diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 699209654f8..1dcfca367db 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -878,19 +878,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))
@@ -1252,13 +1241,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;
@@ -1287,28 +1269,9 @@ 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.
- The direct edge was possibly redirected to the clone with a different
- 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))
- {
- if (dump_file)
- fprintf (dump_file, "Not expanding speculative call of %s -> %s\n"
- "Type mismatch.\n",
- e->caller->dump_name (),
- e->callee->dump_name ());
- e = e->resolve_speculation ();
- /* We are producing the final function body and will throw away the
- callgraph edges really soon. Reset the counts/frequencies to
- keep verifier happy in the case of roundoff errors. */
- e->count = gimple_bb (e->call_stmt)->count;
- }
- /* Expand speculation into GIMPLE code. */
else
{
+ /* Expand speculation into GIMPLE code. */
if (dump_file)
{
fprintf (dump_file,
@@ -3635,102 +3598,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 0c5a6960d10..8989f3119ac 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -2445,8 +2445,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/ipa-inline.c b/gcc/ipa-inline.c
index dacec7d401b..9c48bbd0411 100644
--- a/gcc/ipa-inline.c
+++ b/gcc/ipa-inline.c
@@ -2913,14 +2913,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 3a6e21bf013..ba1c5d8a00d 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -3492,11 +3492,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..789309c533e 100644
--- a/gcc/value-prof.c
+++ b/gcc/value-prof.c
@@ -1245,25 +1245,6 @@ 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. */
-
-bool
-check_ic_target (gcall *call_stmt, struct cgraph_node *target)
-{
- if (gimple_check_call_matching_types (call_stmt, target->decl, true))
- return true;
-
- if (dump_enabled_p ())
- dump_printf_loc (MSG_MISSED_OPTIMIZATION, call_stmt,
- "Skipping target %s with mismatching types for icall\n",
- target->name ());
- return false;
-}
-
/* Do transformation
if (actual_callee_address == address_of_most_common_function/method)
@@ -1456,17 +1437,6 @@ gimple_ic_transform (gimple_stmt_iterator *gsi)
return false;
}
- if (!check_ic_target (stmt, direct_call))
- {
- if (dump_enabled_p ())
- dump_printf_loc (MSG_MISSED_OPTIMIZATION, stmt,
- "Indirect call -> direct call %T => %T "
- "transformation skipped because of type mismatch: %G",
- gimple_call_fn (stmt), direct_call->decl, stmt);
- gimple_remove_histogram_value (cfun, stmt, histogram);
- return false;
- }
-
if (dump_enabled_p ())
{
dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, stmt,
--
2.23.0
