| Message ID | 20140606070025.GB65215@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
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))
On 09/15/14 01:20, Ilya Enkovich wrote: >>> >> >> 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. OK. jeff
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))