On Tue, Jun 3, 2014 at 3:27 PM, Ilya Enkovich <enkovich....@gmail.com> wrote:
> 2014-06-03 15:56 GMT+04:00 Richard Biener <richard.guent...@gmail.com>:
>> On Tue, Jun 3, 2014 at 1:36 PM, Ilya Enkovich <enkovich....@gmail.com> wrote:
>>> 2014-06-03 13:45 GMT+04:00 Richard Biener <richard.guent...@gmail.com>:
>>>> On Tue, Jun 3, 2014 at 9:23 AM, Ilya Enkovich <enkovich....@gmail.com>
>>>> 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 <ilya.enkov...@intel.com>
>>>>>
>>>>> * 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 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))
