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

[Richard Biener <richard dot guenther at gmail dot 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?

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

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