[PATCH, Pointer Bounds Checker 25/x] DCE

Ilya Enkovich enkovich.gnu@gmail.com
Mon Sep 15 07:20:00 GMT 2014


Ping

2014-06-06 11:00 GMT+04:00 Ilya Enkovich <enkovich.gnu@gmail.com>:
> On 03 Jun 17:27, Ilya Enkovich wrote:
>> 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
>>
>
> Here is a fixed version.
>
> Bootstrapped and tested on linux_x86-64.
>
> Thanks,
> Ilya
> --
> gcc/
>
> 2014-06-05  Ilya Enkovich  <ilya.enkovich@intel.com>
>
>         * tree-ssa-dce.c: Include tree-chkp.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..f186706 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 "tree-chkp.h"
>
>  static struct stmt_stats
>  {
> @@ -778,7 +779,22 @@ 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;
> +               {
> +                 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))
> +                         && TREE_CODE (bounds) == SSA_NAME
> +                         && (bounds_def_stmt = SSA_NAME_DEF_STMT (bounds))
> +                         && is_gimple_call (bounds_def_stmt)
> +                         && chkp_gimple_call_builtin_p (bounds_def_stmt,
> +                                                        BUILT_IN_CHKP_BNDRET)
> +                         && gimple_call_arg (bounds_def_stmt, 0) == ptr))
> +                   continue;
> +               }
>             }
>
>           FOR_EACH_SSA_TREE_OPERAND (use, stmt, iter, SSA_OP_USE)
> @@ -1204,6 +1220,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.  */
> @@ -1233,7 +1266,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.  */
> +                 && !chkp_gimple_call_builtin_p (stmt, BUILT_IN_CHKP_BNDRET))
>                 {
>                   something_changed = true;
>                   if (dump_file && (dump_flags & TDF_DETAILS))



More information about the Gcc-patches mailing list