2014-06-03 17:41 GMT+04:00 Richard Biener <[email protected]>:
> On Tue, Jun 3, 2014 at 3:27 PM, Ilya Enkovich <[email protected]> wrote:
>> 2014-06-03 15:56 GMT+04:00 Richard Biener <[email protected]>:
>>> On Tue, Jun 3, 2014 at 1:36 PM, Ilya Enkovich <[email protected]>
>>> wrote:
>>>> 2014-06-03 13:45 GMT+04:00 Richard Biener <[email protected]>:
>>>>> On Tue, Jun 3, 2014 at 9:23 AM, Ilya Enkovich <[email protected]>
>>>>> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> This patch adjusts alloc-free removal algorithm in DCE to take into
>>>>>> account BUILT_IN_CHKP_BNDRET call returning bounds of allocated memory.
>>>>>>
>>>>>> Bootstrapped and tested on linux-x86_64.
>>>>>>
>>>>>> Thanks,
>>>>>> Ilya
>>>>>> --
>>>>>> gcc/
>>>>>>
>>>>>> 2014-06-03 Ilya Enkovich <[email protected]>
>>>>>>
>>>>>> * tree-ssa-dce.c: Include target.h.
>>>>>> (propagate_necessity): For free call fed by alloc check
>>>>>> bounds are also provided by the same alloc.
>>>>>> (eliminate_unnecessary_stmts): Handle BUILT_IN_CHKP_BNDRET
>>>>>> used by free calls.
>>>>>>
>>>>>> diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c
>>>>>> index 13a71ce..59a0b71 100644
>>>>>> --- a/gcc/tree-ssa-dce.c
>>>>>> +++ b/gcc/tree-ssa-dce.c
>>>>>> @@ -73,6 +73,7 @@ along with GCC; see the file COPYING3. If not see
>>>>>> #include "flags.h"
>>>>>> #include "cfgloop.h"
>>>>>> #include "tree-scalar-evolution.h"
>>>>>> +#include "target.h"
>>>>>>
>>>>>> static struct stmt_stats
>>>>>> {
>>>>>> @@ -778,7 +779,23 @@ propagate_necessity (bool aggressive)
>>>>>> && DECL_BUILT_IN_CLASS (def_callee) == BUILT_IN_NORMAL
>>>>>> && (DECL_FUNCTION_CODE (def_callee) == BUILT_IN_MALLOC
>>>>>> || DECL_FUNCTION_CODE (def_callee) ==
>>>>>> BUILT_IN_CALLOC))
>>>>>> - continue;
>>>>>> + {
>>>>>> + tree retfndecl
>>>>>> + = targetm.builtin_chkp_function
>>>>>> (BUILT_IN_CHKP_BNDRET);
>>>>>> + gimple bounds_def_stmt;
>>>>>> + tree bounds;
>>>>>> +
>>>>>> + /* For instrumented calls we should also check used
>>>>>> + bounds are returned by the same allocation call. */
>>>>>> + if (!gimple_call_with_bounds_p (stmt)
>>>>>> + || ((bounds = gimple_call_arg (stmt, 1))
>>>>>
>>>>> Please provide an abstraction for this - I seem to recall seeing multiple
>>>>> (variants) of this. Esp. repeated calls to the target hook above look
>>>>> expensive to me (generally it will not be needed).
>>>>>
>>>>> I'd like to see gimple_call_bndarg (stmt) to get rid of the "magic" 1 and
>>>>> I'd
>>>>> like to see sth similar to gimple_call_builtin_p, for example
>>>>> gimple_call_chkp_p (stmt, BUILT_IN_CHKP_BNDRET) doing the
>>>>> target hook invocation and the fndecl check.
>>>>
>>>> I do not see how to get rid of constant in gimple_call_arg call here.
>>>> What semantics does proposed gimple_call_bndarg have? It has to return
>>>> the first bounds arg but it's not clear from its name and function
>>>> seems to be very specialized.
>>>
>>> Ah, ok. I see now, so the code is ok as-is.
>>>
>>>>>
>>>>>> + && TREE_CODE (bounds) == SSA_NAME
>>>>>
>>>>> What about constant bounds? That shouldn't make the call necessary
>>>>> either?
>>>>
>>>> Yep, it would be useless.
>>>
>>> So please fix.
>>>
>>>>>
>>>>>> + && (bounds_def_stmt = SSA_NAME_DEF_STMT
>>>>>> (bounds))
>>>>>> + && is_gimple_call (bounds_def_stmt)
>>>>>> + && gimple_call_fndecl (bounds_def_stmt) ==
>>>>>> retfndecl
>>>>>> + && gimple_call_arg (bounds_def_stmt, 0) ==
>>>>>> ptr))
>>>>>> + continue;
>>>>>
>>>>> So this all becomes
>>>>>
>>>>> if (!gimple_call_with_bounds_p (stmt)
>>>>> || ((bounds = gimple_call_bndarg (stmt))
>>>>> && TREE_CODE (bounds) == SSA_NAME
>>>>> && (bounds_def_stmt = SSA_NAME_DEF_STMT
>>>>> (bounds))
>>>>> && is_gimple_call (bounds_def_stmt)
>>>>> && gimple_call_chkp_p (bounds_def_stmt,
>>>>> BUILT_IN_CHKP_BNDRET)
>>>>> ...
>>>>>
>>>>> btw, I wonder how BUILT_IN_CHKP_BNDRET is in scope here but you
>>>>> need a target hook to compare the fndecl? Why isn't it enough to
>>>>> just check DECL_FUNCTION_CODE and DECL_BUILT_IN?
>>>>
>>>> Call is required because builtins for Pointer Bounds Checker are
>>>> provided by target depending on available features. That is why
>>>> gimple_call_builtin_p is not used. I may add new interface function
>>>> into tree-chkp.h as you propose (e.g. chkp_gimple_call_builtin_p). But
>>>> I do not see how it may speed-up the code. New function would still
>>>> need to switch by function code and compare with proper decl which is
>>>> basically what we have with target hook.
>>>
>>> I don't understand - does this mean the builtin calls are in the GIMPLE
>>> IL even though the target doesn't have the feature? Isn't the presence
>>> of the call enough?
>>
>> Call is generated using function decl provided by target. Therefore we
>> do not know its code and has to compare using fndecl comparison.
>
> So they are target specific builtins after all? IMNSHO it looks wrong
> that the middle-end has to care about them in that case. Why can't
> the builtins itself be target independent?
It may be target specific builtin or a generic one depending on what
is used for your current target. With something like
chkp_gimple_call_builtin_p checks for checker builtins should look
better.
Ilya
>
>>>> It is possible to make faster if use the fact that all chkp builtins
>>>> codes (normal, not target ones) are consequent. Then we may just check
>>>> the range and use code as an index in array of chkp fndecls to compare
>>>> decls. Would it be OK to rely on builtin codes numbering?
>>>
>>> Yes, but that's not my point here. In the above code the target hook
>>> is called all the time, even if !gimple_call_with_bounds_p (). Providing
>>> a suitable abstraction (or simply relying on DECL_FUNCTION_CODE)
>>> should fix that.
>>
>> Got it. Will fix.
>>
>> Thanks,
>> Ilya
>>
>>>
>>> Richard.
>>>
>>>> Ilya
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> + }
>>>>>> }
>>>>>>
>>>>>> FOR_EACH_SSA_TREE_OPERAND (use, stmt, iter, SSA_OP_USE)
>>>>>> @@ -1204,6 +1221,23 @@ eliminate_unnecessary_stmts (void)
>>>>>> && !gimple_plf (def_stmt, STMT_NECESSARY))
>>>>>> gimple_set_plf (stmt, STMT_NECESSARY, false);
>>>>>> }
>>>>>> + /* We did not propagate necessity for free calls fed
>>>>>> + by allocation function to allow unnecessary
>>>>>> + alloc-free sequence elimination. For instrumented
>>>>>> + calls it also means we did not mark bounds producer
>>>>>> + as necessary and it is time to do it in case free
>>>>>> + call is not removed. */
>>>>>> + if (gimple_call_with_bounds_p (stmt))
>>>>>> + {
>>>>>> + gimple bounds_def_stmt;
>>>>>> + tree bounds = gimple_call_arg (stmt, 1);
>>>>>> + gcc_assert (TREE_CODE (bounds) == SSA_NAME);
>>>>>> + bounds_def_stmt = SSA_NAME_DEF_STMT (bounds);
>>>>>> + if (bounds_def_stmt
>>>>>> + && !gimple_plf (bounds_def_stmt, STMT_NECESSARY))
>>>>>> + gimple_set_plf (bounds_def_stmt, STMT_NECESSARY,
>>>>>> + gimple_plf (stmt, STMT_NECESSARY));
>>>>>> + }
>>>>>> }
>>>>>>
>>>>>> /* If GSI is not necessary then remove it. */
>>>>>> @@ -1216,6 +1250,7 @@ eliminate_unnecessary_stmts (void)
>>>>>> else if (is_gimple_call (stmt))
>>>>>> {
>>>>>> tree name = gimple_call_lhs (stmt);
>>>>>> + tree retfn = targetm.builtin_chkp_function
>>>>>> (BUILT_IN_CHKP_BNDRET);
>>>>>>
>>>>>> notice_special_calls (stmt);
>>>>>>
>>>>>> @@ -1233,7 +1268,9 @@ eliminate_unnecessary_stmts (void)
>>>>>> && DECL_FUNCTION_CODE (call) != BUILT_IN_CALLOC
>>>>>> && DECL_FUNCTION_CODE (call) != BUILT_IN_ALLOCA
>>>>>> && (DECL_FUNCTION_CODE (call)
>>>>>> - != BUILT_IN_ALLOCA_WITH_ALIGN))))
>>>>>> + != BUILT_IN_ALLOCA_WITH_ALIGN)))
>>>>>> + /* Avoid doing so for bndret calls for the same
>>>>>> reason. */
>>>>>> + && (!retfn || gimple_call_fndecl (stmt) != retfn))
>>>>>> {
>>>>>> something_changed = true;
>>>>>> if (dump_file && (dump_flags & TDF_DETAILS))
