* FDO usability -- sanity check indirect call target
@ 2011-04-08 6:06 Xinliang David Li
2011-04-08 9:27 ` Richard Guenther
0 siblings, 1 reply; 6+ messages in thread
From: Xinliang David Li @ 2011-04-08 6:06 UTC (permalink / raw)
To: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 800 bytes --]
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?
Thanks,
David
2011-04-07 Xinliang David Li <davidxl@google.com>
* value-profile.c (function_decl_num_args): New function.
(check_ic_target): New function.
(gimple_ic_transform): Sanity check indirect call target.
[-- Attachment #2: check_ic.p --]
[-- Type: application/octet-stream, Size: 2552 bytes --]
Index: value-prof.c
===================================================================
--- value-prof.c (revision 171959)
+++ value-prof.c (working copy)
@@ -1090,6 +1090,62 @@ find_func_by_pid (int pid)
return pid_map [pid];
}
+/* Counts the number of arguments in the function_decl FN_DECL.
+
+ Returns the number of arguments. */
+static unsigned
+function_decl_num_args (tree fn_decl)
+{
+ tree arg;
+ unsigned count = 0;
+ for (arg = DECL_ARGUMENTS (fn_decl); arg; arg = TREE_CHAIN (arg))
+ count++;
+ return count;
+}
+
+/* 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)
+{
+ tree tgt_decl;
+ /* Return types. */
+ tree icall_type, tgt_type;
+ tree call_expr;
+
+ tgt_decl = target->decl;
+ tgt_type = TREE_TYPE (TREE_TYPE (tgt_decl));
+
+ call_expr = gimple_call_fn (call_stmt);
+ icall_type = TREE_TYPE (call_expr);
+ if (POINTER_TYPE_P (icall_type))
+ icall_type = TREE_TYPE (icall_type);
+ icall_type = TREE_TYPE (icall_type);
+
+ if (TREE_CODE (icall_type) != TREE_CODE (tgt_type)
+ || TYPE_MODE (icall_type) != TYPE_MODE (tgt_type))
+ return false;
+
+ /* Verify that CALL_STMT has the same number of arguments as TGT_TYPE. */
+ if (gimple_call_num_args (call_stmt) != function_decl_num_args (tgt_decl))
+ return false;
+
+ /* Record types should have matching sizes. */
+ if (TREE_CODE (icall_type) == RECORD_TYPE
+ && TYPE_SIZE (icall_type) != NULL_TREE
+ && TYPE_SIZE (tgt_type) != NULL_TREE
+ && TREE_CODE (TYPE_SIZE (icall_type)) == INTEGER_CST
+ && (TREE_CODE (TYPE_SIZE (tgt_type)) != INTEGER_CST ||
+ !tree_int_cst_equal (TYPE_SIZE (icall_type), TYPE_SIZE (tgt_type))))
+ return false;
+
+ return true;
+}
+
/* Do transformation
if (actual_callee_address == address_of_most_common_function/method)
@@ -1271,6 +1327,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)
@@ -1751,4 +1810,3 @@ gimple_find_values_to_profile (histogram
}
}
}
-
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: FDO usability -- sanity check indirect call target
2011-04-08 6:06 FDO usability -- sanity check indirect call target Xinliang David Li
@ 2011-04-08 9:27 ` Richard Guenther
2011-04-08 21:42 ` Xinliang David Li
0 siblings, 1 reply; 6+ messages in thread
From: Richard Guenther @ 2011-04-08 9:27 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches
On Fri, Apr 8, 2011 at 8:06 AM, Xinliang David Li <davidxl@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 <davidxl@google.com>
>
> * value-profile.c (function_decl_num_args): New function.
> (check_ic_target): New function.
> (gimple_ic_transform): Sanity check indirect call target.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: FDO usability -- sanity check indirect call target
2011-04-08 9:27 ` Richard Guenther
@ 2011-04-08 21:42 ` Xinliang David Li
2011-04-09 16:35 ` Xinliang David Li
2011-04-11 9:09 ` Richard Guenther
0 siblings, 2 replies; 6+ messages in thread
From: Xinliang David Li @ 2011-04-08 21:42 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 1783 bytes --]
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 <davidxl@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.guenther@gmail.com> wrote:
> On Fri, Apr 8, 2011 at 8:06 AM, Xinliang David Li <davidxl@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 <davidxl@google.com>
>>
>> * value-profile.c (function_decl_num_args): New function.
>> (check_ic_target): New function.
>> (gimple_ic_transform): Sanity check indirect call target.
>>
>
[-- Attachment #2: check_ic2.p --]
[-- Type: text/x-pascal, Size: 4699 bytes --]
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 */
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: FDO usability -- sanity check indirect call target
2011-04-08 21:42 ` Xinliang David Li
@ 2011-04-09 16:35 ` Xinliang David Li
2011-04-11 6:06 ` Xinliang David Li
2011-04-11 9:09 ` Richard Guenther
1 sibling, 1 reply; 6+ messages in thread
From: Xinliang David Li @ 2011-04-09 16:35 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
SPEC06 build with FDO is ok with the patch -- no abnormality was found.
David
On Fri, Apr 8, 2011 at 2:42 PM, Xinliang David Li <davidxl@google.com> wrote:
> 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 <davidxl@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.guenther@gmail.com> wrote:
>> On Fri, Apr 8, 2011 at 8:06 AM, Xinliang David Li <davidxl@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 <davidxl@google.com>
>>>
>>> * value-profile.c (function_decl_num_args): New function.
>>> (check_ic_target): New function.
>>> (gimple_ic_transform): Sanity check indirect call target.
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: FDO usability -- sanity check indirect call target
2011-04-09 16:35 ` Xinliang David Li
@ 2011-04-11 6:06 ` Xinliang David Li
0 siblings, 0 replies; 6+ messages in thread
From: Xinliang David Li @ 2011-04-11 6:06 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
Is this patch OK?
Thanks,
David
On Sat, Apr 9, 2011 at 9:34 AM, Xinliang David Li <davidxl@google.com> wrote:
> SPEC06 build with FDO is ok with the patch -- no abnormality was found.
>
> David
>
> On Fri, Apr 8, 2011 at 2:42 PM, Xinliang David Li <davidxl@google.com> wrote:
>> 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 <davidxl@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.guenther@gmail.com> wrote:
>>> On Fri, Apr 8, 2011 at 8:06 AM, Xinliang David Li <davidxl@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 <davidxl@google.com>
>>>>
>>>> * value-profile.c (function_decl_num_args): New function.
>>>> (check_ic_target): New function.
>>>> (gimple_ic_transform): Sanity check indirect call target.
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: FDO usability -- sanity check indirect call target
2011-04-08 21:42 ` Xinliang David Li
2011-04-09 16:35 ` Xinliang David Li
@ 2011-04-11 9:09 ` Richard Guenther
1 sibling, 0 replies; 6+ messages in thread
From: Richard Guenther @ 2011-04-11 9:09 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches
On Fri, Apr 8, 2011 at 11:42 PM, Xinliang David Li <davidxl@google.com> wrote:
> 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).
This one is ok.
Thanks,
Richard.
> Thanks,
>
> David
>
> 2011-04-07 Xinliang David Li <davidxl@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.guenther@gmail.com> wrote:
>> On Fri, Apr 8, 2011 at 8:06 AM, Xinliang David Li <davidxl@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 <davidxl@google.com>
>>>
>>> * value-profile.c (function_decl_num_args): New function.
>>> (check_ic_target): New function.
>>> (gimple_ic_transform): Sanity check indirect call target.
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-04-11 9:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-08 6:06 FDO usability -- sanity check indirect call target Xinliang David Li
2011-04-08 9:27 ` Richard Guenther
2011-04-08 21:42 ` Xinliang David Li
2011-04-09 16:35 ` Xinliang David Li
2011-04-11 6:06 ` Xinliang David Li
2011-04-11 9:09 ` Richard Guenther
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).