This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, Pointer Bounds Checker 25/x] DCE


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

>
>> 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))


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]