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


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


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