Please review this patch. Regression test is ok. I will do more
application testing to make sure the check is not too strict
(filtering out legal ic targets).
Thanks,
David
2011-04-07 Xinliang David Li <davi...@google.com>
* value-profile.c (function_decl_num_args): New function.
(check_ic_target): New function.
(gimple_ic_transform): Sanity check indirect call target.
* gimple-low.c (gimple_check_call_args): Interface change.
(gimple_check_call_matching_types): New function.
* tree-inline.c (tree_can_inline_p): Call new function.
On Fri, Apr 8, 2011 at 2:27 AM, Richard Guenther
<richard.guent...@gmail.com> wrote:
> On Fri, Apr 8, 2011 at 8:06 AM, Xinliang David Li <davi...@google.com> wrote:
>> Hi, due to race conditions, it is common that the value profile
>> information for an indirect call site is 'corrupted' -- resulting in
>> false target function to be recorded. The value profile transformation
>> won't cause runtime problem as the path will never be executed,
>> however it may cause compile time ICE because of the incompatible
>> signature of the callee target. The attached patch does minimal sanity
>> check to make compiler happy in such cases. The fix was tested with
>> lots of MT programs and works really well in practice.
>>
>> Ok for trunk after testing?
>
> Please instead refactor and re-use gimple_check_call_args ().
> Also look at tree_can_inline_p () which has code to deal with
> result mismatches.
>
> Richard.
>
>> Thanks,
>>
>> David
>>
>> 2011-04-07 Xinliang David Li <davi...@google.com>
>>
>> * value-profile.c (function_decl_num_args): New function.
>> (check_ic_target): New function.
>> (gimple_ic_transform): Sanity check indirect call target.
>>
>
Index: value-prof.c
===================================================================
--- value-prof.c (revision 172211)
+++ value-prof.c (working copy)
@@ -1090,6 +1090,25 @@ find_func_by_pid (int pid)
return pid_map [pid];
}
+/* 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. */
+
+static bool
+check_ic_target (gimple call_stmt, struct cgraph_node *target)
+{
+ location_t locus;
+ if (gimple_check_call_matching_types (call_stmt, target->decl))
+ return true;
+
+ locus = gimple_location (call_stmt);
+ inform (locus, "Skipping target %s with mismatching types for icall ",
+ cgraph_node_name (target));
+ return false;
+}
+
/* Do transformation
if (actual_callee_address == address_of_most_common_function/method)
@@ -1268,6 +1287,9 @@ gimple_ic_transform (gimple stmt)
if (direct_call == NULL)
return false;
+ if (!check_ic_target (stmt, direct_call))
+ return false;
+
modify = gimple_ic (stmt, direct_call, prob, count, all);
if (dump_file)
@@ -1748,4 +1770,3 @@ gimple_find_values_to_profile (histogram
}
}
}
-
Index: gimple-low.c
===================================================================
--- gimple-low.c (revision 172211)
+++ gimple-low.c (working copy)
@@ -208,20 +208,20 @@ struct gimple_opt_pass pass_lower_cf =
};
+
/* 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. */
-bool
-gimple_check_call_args (gimple stmt)
+static bool
+gimple_check_call_args (gimple stmt, tree fndecl)
{
- tree fndecl, parms, p;
+ tree parms, p;
unsigned int i, nargs;
nargs = gimple_call_num_args (stmt);
/* Get argument types for verification. */
- fndecl = gimple_call_fndecl (stmt);
if (fndecl)
parms = TYPE_ARG_TYPES (TREE_TYPE (fndecl));
else
@@ -272,6 +272,25 @@ gimple_check_call_args (gimple stmt)
return true;
}
+/* Verify if the type of the argument and lhs of CALL_STMT matches
+ that of the function declaration CALLEE.
+ If we cannot verify this or there is a mismatch, return false. */
+
+bool
+gimple_check_call_matching_types (gimple call_stmt, tree callee)
+{
+ 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))
+ return false;
+ return true;
+}
/* Lower sequence SEQ. Unlike gimplification the statements are not relowered
when they are changed -- if this has to be done, the lowering routine must
Index: tree-inline.c
===================================================================
--- tree-inline.c (revision 172211)
+++ tree-inline.c (working copy)
@@ -5325,7 +5325,7 @@ tree_can_inline_p (struct cgraph_edge *e
return false;
}
#endif
- tree caller, callee, lhs;
+ tree caller, callee;
caller = e->caller->decl;
callee = e->callee->decl;
@@ -5352,13 +5352,7 @@ tree_can_inline_p (struct cgraph_edge *e
/* Do not inline calls where we cannot triviall work around mismatches
in argument or return types. */
if (e->call_stmt
- && ((DECL_RESULT (callee)
- && !DECL_BY_REFERENCE (DECL_RESULT (callee))
- && (lhs = gimple_call_lhs (e->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 (e->call_stmt)))
+ && !gimple_check_call_matching_types (e->call_stmt, callee))
{
e->inline_failed = CIF_MISMATCHED_ARGUMENTS;
if (e->call_stmt)
Index: tree-flow.h
===================================================================
--- tree-flow.h (revision 172211)
+++ tree-flow.h (working copy)
@@ -515,7 +515,7 @@ extern void record_vars_into (tree, tree
extern void record_vars (tree);
extern bool gimple_seq_may_fallthru (gimple_seq);
extern bool gimple_stmt_may_fallthru (gimple);
-extern bool gimple_check_call_args (gimple);
+extern bool gimple_check_call_matching_types (gimple, tree);
/* In tree-ssa.c */
