[PATCH, Pointer Bounds Checker 23/x] Function split

[PATCH, Pointer Bounds Checker 23/x] Function split

[Ilya Enkovich]

Hi,

This patch does not allow splitting in case bounds are returned until retutrned bounds are supported.  It also propagates instrumentation marks for generated call and function.

Bootstrapped and tested on linux-x86_64.

Thanks,
Ilya
--
gcc/

2014-06-03  Ilya Enkovich  <ilya.enkovich@intel.com>

	* ipa-split.c: Include tree-chkp.h.
	(consider_split): Do not split when return bounds.
	(split_function): Propagate Pointer Bounds Checker
	instrumentation marks.


diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
index 38bd883..edf322f 100644
--- a/gcc/ipa-split.c
+++ b/gcc/ipa-split.c
@@ -110,6 +110,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimple-pretty-print.h"
 #include "ipa-inline.h"
 #include "cfgloop.h"
+#include "tree-chkp.h"
 
 /* Per basic block info.  */
 
@@ -496,6 +497,19 @@ consider_split (struct split_point *current, bitmap non_ssa_vars,
   if (!VOID_TYPE_P (TREE_TYPE (current_function_decl)))
     call_overhead += estimate_move_cost (TREE_TYPE (current_function_decl));
 
+  /* Currently returned value is processed but returned bounds
+     are not processed.  It results in bounds in return statement
+     with no definition.  Forbid split until returned bounds are
+     supported.  */
+  if (chkp_function_instrumented_p (current_function_decl)
+      && chkp_type_has_pointer (TREE_TYPE (TREE_TYPE (current_function_decl))))
+    {
+      if (dump_file && (dump_flags & TDF_DETAILS))
+	fprintf (dump_file,
+		 "  Refused: need to return bounds\n");
+      return;
+    }
+
   if (current->split_size <= call_overhead)
     {
       if (dump_file && (dump_flags & TDF_DETAILS))
@@ -1096,6 +1110,7 @@ split_function (struct split_point *split_point)
   edge_iterator ei;
   tree retval = NULL, real_retval = NULL;
   bool split_part_return_p = false;
+  bool with_bounds = chkp_function_instrumented_p (current_function_decl);
   gimple last_stmt = NULL;
   unsigned int i;
   tree arg, ddef;
@@ -1248,6 +1263,12 @@ split_function (struct split_point *split_point)
       DECL_BUILT_IN_CLASS (node->decl) = NOT_BUILT_IN;
       DECL_FUNCTION_CODE (node->decl) = (enum built_in_function) 0;
     }
+
+  /* If the original function is instrumented then it's
+     part is also instrumented.  */
+  if (with_bounds)
+    chkp_function_mark_instrumented (node->decl);
+
   /* If the original function is declared inline, there is no point in issuing
      a warning for the non-inlinable part.  */
   DECL_NO_INLINE_WARNING_P (node->decl) = 1;
@@ -1282,6 +1303,7 @@ split_function (struct split_point *split_point)
 	args_to_pass[i] = arg;
       }
   call = gimple_build_call_vec (node->decl, args_to_pass);
+  gimple_call_set_with_bounds (call, with_bounds);
   gimple_set_block (call, DECL_INITIAL (current_function_decl));
   args_to_pass.release ();
 

Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Jeff Law]

On 06/03/14 01:10, Ilya Enkovich wrote:
> Hi,
>
> This patch does not allow splitting in case bounds are returned until retutrned bounds are supported.  It also propagates instrumentation marks for generated call and function.
>
> Bootstrapped and tested on linux-x86_64.
>
> Thanks,
> Ilya
> --
> gcc/
>
> 2014-06-03  Ilya Enkovich  <ilya.enkovich@intel.com>
>
> 	* ipa-split.c: Include tree-chkp.h.
> 	(consider_split): Do not split when return bounds.
> 	(split_function): Propagate Pointer Bounds Checker
> 	instrumentation marks.
It's a hack.  There's no reason we can't support this.  So I'll approve 
on the condition that you do look at removing this limitation in the future.

jeff

Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Ilya Enkovich]

2014-06-04 11:15 GMT+04:00 Jeff Law <law@redhat.com>:
> On 06/03/14 01:10, Ilya Enkovich wrote:
>>
>> Hi,
>>
>> This patch does not allow splitting in case bounds are returned until
>> retutrned bounds are supported.  It also propagates instrumentation marks
>> for generated call and function.
>>
>> Bootstrapped and tested on linux-x86_64.
>>
>> Thanks,
>> Ilya
>> --
>> gcc/
>>
>> 2014-06-03  Ilya Enkovich  <ilya.enkovich@intel.com>
>>
>>         * ipa-split.c: Include tree-chkp.h.
>>         (consider_split): Do not split when return bounds.
>>         (split_function): Propagate Pointer Bounds Checker
>>         instrumentation marks.
>
> It's a hack.  There's no reason we can't support this.  So I'll approve on
> the condition that you do look at removing this limitation in the future.

I looked into it and did not find an easy way to fix damaged return.
Will definitely work more on this later. Any help from knowledgeable
person (Jan Hubicka?) would be great here.

Ilya
>
> jeff
>

Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Ilya Enkovich]

On 04 Jun 01:15, Jeff Law wrote:
> On 06/03/14 01:10, Ilya Enkovich wrote:
> >Hi,
> >
> >This patch does not allow splitting in case bounds are returned until retutrned bounds are supported.  It also propagates instrumentation marks for generated call and function.
> >
> >Bootstrapped and tested on linux-x86_64.
> >
> >Thanks,
> >Ilya
> >--
> >gcc/
> >
> >2014-06-03  Ilya Enkovich  <ilya.enkovich@intel.com>
> >
> >	* ipa-split.c: Include tree-chkp.h.
> >	(consider_split): Do not split when return bounds.
> >	(split_function): Propagate Pointer Bounds Checker
> >	instrumentation marks.
> It's a hack.  There's no reason we can't support this.  So I'll
> approve on the condition that you do look at removing this
> limitation in the future.
> 
> jeff
> 

I did some work for function splitting and now patch cover more cases.  Now returned bounds are supported but it is not allowed to split producers of returned pointer and its bounds.  Is it OK?

Thanks,
Ilya
--
2014-08-15  Ilya Enkovich  <ilya.enkovich@intel.com>

	* ipa-split.c: Include tree-chkp.h.
	(find_retbnd): New.
	(consider_split): Do not split retbnd and retval
	producers.
	(split_function): Propagate Pointer Bounds Checker
	instrumentation marks and handle returned bounds.


diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
index 2af3a93..f8ecaf7 100644
--- a/gcc/ipa-split.c
+++ b/gcc/ipa-split.c
@@ -110,6 +110,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimple-pretty-print.h"
 #include "ipa-inline.h"
 #include "cfgloop.h"
+#include "tree-chkp.h"
 
 /* Per basic block info.  */
 
@@ -151,6 +152,7 @@ struct split_point best_split_point;
 static bitmap forbidden_dominators;
 
 static tree find_retval (basic_block return_bb);
+static tree find_retbnd (basic_block return_bb);
 
 /* Callback for walk_stmt_load_store_addr_ops.  If T is non-SSA automatic
    variable, check it if it is present in bitmap passed via DATA.  */
@@ -387,6 +389,7 @@ consider_split (struct split_point *current, bitmap non_ssa_vars,
   unsigned int i;
   int incoming_freq = 0;
   tree retval;
+  tree retbnd;
   bool back_edge = false;
 
   if (dump_file && (dump_flags & TDF_DETAILS))
@@ -588,6 +591,32 @@ consider_split (struct split_point *current, bitmap non_ssa_vars,
   else
     current->split_part_set_retval = true;
 
+  /* See if retbnd used by return bb is computed by header or split part.  */
+  retbnd = find_retbnd (return_bb);
+  if (retbnd)
+    {
+      bool split_part_set_retbnd
+	= (!SSA_NAME_IS_DEFAULT_DEF (retbnd)
+	   && (bitmap_bit_p (current->split_bbs,
+			     gimple_bb (SSA_NAME_DEF_STMT (retbnd))->index)
+	       || gimple_bb (SSA_NAME_DEF_STMT (retbnd)) == return_bb));
+
+      /* If we have both return value and bounds then keep their definitions
+	 in a single function.  We use SSA names to link returned bounds and
+	 value and therefore do not handle cases when result is passed by
+	 reference (which should not be our case anyway since bounds are
+	 returned for pointers only).  */
+      if ((DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))
+	   && current->split_part_set_retval)
+	  || split_part_set_retbnd != current->split_part_set_retval)
+	{
+	  if (dump_file && (dump_flags & TDF_DETAILS))
+	    fprintf (dump_file,
+		     "  Refused: split point splits return value and bounds\n");
+	  return;
+	}
+    }
+
   /* split_function fixes up at most one PHI non-virtual PHI node in return_bb,
      for the return value.  If there are other PHIs, give up.  */
   if (return_bb != EXIT_BLOCK_PTR_FOR_FN (cfun))
@@ -710,6 +739,18 @@ find_retval (basic_block return_bb)
   return NULL;
 }
 
+/* Given return basic block RETURN_BB, see where return bounds are really
+   stored.  */
+static tree
+find_retbnd (basic_block return_bb)
+{
+  gimple_stmt_iterator bsi;
+  for (bsi = gsi_start_bb (return_bb); !gsi_end_p (bsi); gsi_next (&bsi))
+    if (gimple_code (gsi_stmt (bsi)) == GIMPLE_RETURN)
+      return gimple_return_retbnd (gsi_stmt (bsi));
+  return NULL;
+}
+
 /* Callback for walk_stmt_load_store_addr_ops.  If T is non-SSA automatic
    variable, mark it as used in bitmap passed via DATA.
    Return true when access to T prevents splitting the function.  */
@@ -1095,8 +1136,9 @@ split_function (struct split_point *split_point)
   gimple call;
   edge e;
   edge_iterator ei;
-  tree retval = NULL, real_retval = NULL;
+  tree retval = NULL, real_retval = NULL, retbnd = NULL;
   bool split_part_return_p = false;
+  bool with_bounds = chkp_function_instrumented_p (current_function_decl);
   gimple last_stmt = NULL;
   unsigned int i;
   tree arg, ddef;
@@ -1245,6 +1287,12 @@ split_function (struct split_point *split_point)
       DECL_BUILT_IN_CLASS (node->decl) = NOT_BUILT_IN;
       DECL_FUNCTION_CODE (node->decl) = (enum built_in_function) 0;
     }
+
+  /* If the original function is instrumented then it's
+     part is also instrumented.  */
+  if (with_bounds)
+    chkp_function_mark_instrumented (node->decl);
+
   /* If the original function is declared inline, there is no point in issuing
      a warning for the non-inlinable part.  */
   DECL_NO_INLINE_WARNING_P (node->decl) = 1;
@@ -1279,6 +1327,7 @@ split_function (struct split_point *split_point)
 	args_to_pass[i] = arg;
       }
   call = gimple_build_call_vec (node->decl, args_to_pass);
+  gimple_call_set_with_bounds (call, with_bounds);
   gimple_set_block (call, DECL_INITIAL (current_function_decl));
   args_to_pass.release ();
 
@@ -1385,6 +1434,7 @@ split_function (struct split_point *split_point)
       if (return_bb != EXIT_BLOCK_PTR_FOR_FN (cfun))
 	{
 	  real_retval = retval = find_retval (return_bb);
+	  retbnd = find_retbnd (return_bb);
 
 	  if (real_retval && split_point->split_part_set_retval)
 	    {
@@ -1429,6 +1479,21 @@ split_function (struct split_point *split_point)
 			  }
 		      update_stmt (gsi_stmt (bsi));
 		    }
+
+		  /* Replace retbnd with new one.  */
+		  if (retbnd)
+		    {
+		      gimple_stmt_iterator bsi;
+		      for (bsi = gsi_start_bb (return_bb); !gsi_end_p (bsi);
+			   gsi_next (&bsi))
+			if (gimple_code (gsi_stmt (bsi)) == GIMPLE_RETURN)
+			  {
+			    retbnd = copy_ssa_name (retbnd, call);
+			    gimple_return_set_retbnd (gsi_stmt (bsi), retbnd);
+			    update_stmt (gsi_stmt (bsi));
+			    break;
+			  }
+		    }
 		}
 	      if (DECL_BY_REFERENCE (DECL_RESULT (current_function_decl)))
 		{
@@ -1450,6 +1515,15 @@ split_function (struct split_point *split_point)
 		      gsi_insert_after (&gsi, cpy, GSI_NEW_STMT);
 		      retval = tem;
 		    }
+		  /* Build bndret call to obtain returned bounds.  */
+		  if (retbnd)
+		    {
+		      tree fndecl
+			= targetm.builtin_chkp_function (BUILT_IN_CHKP_BNDRET);
+		      gimple bndret = gimple_build_call (fndecl, 1, retval);
+		      gimple_call_set_lhs (bndret, retbnd);
+		      gsi_insert_after (&gsi, bndret, GSI_NEW_STMT);
+		    }
 		  gimple_call_set_lhs (call, retval);
 		  update_stmt (call);
 		}
@@ -1468,6 +1542,10 @@ split_function (struct split_point *split_point)
 	    {
 	      retval = DECL_RESULT (current_function_decl);
 
+	      if (chkp_function_instrumented_p (current_function_decl)
+		  && BOUNDED_P (retval))
+		retbnd = create_tmp_reg (pointer_bounds_type_node, NULL);
+
 	      /* We use temporary register to hold value when aggregate_value_p
 		 is false.  Similarly for DECL_BY_REFERENCE we must avoid extra
 		 copy.  */
@@ -1491,6 +1569,15 @@ split_function (struct split_point *split_point)
 	        gimple_call_set_lhs (call, retval);
 	    }
           gsi_insert_after (&gsi, call, GSI_NEW_STMT);
+	  /* Build bndret call to obtain returned bounds.  */
+	  if (retbnd)
+	    {
+	      tree fndecl
+		= targetm.builtin_chkp_function (BUILT_IN_CHKP_BNDRET);
+	      gimple bndret = gimple_build_call (fndecl, 1, retval);
+	      gimple_call_set_lhs (bndret, retbnd);
+	      gsi_insert_after (&gsi, bndret, GSI_NEW_STMT);
+	    }
 	  ret = gimple_build_return (retval);
 	  gsi_insert_after (&gsi, ret, GSI_NEW_STMT);
 	}

Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Jeff Law]

On 08/18/14 09:55, Ilya Enkovich wrote:
> On 04 Jun 01:15, Jeff Law wrote:
>> On 06/03/14 01:10, Ilya Enkovich wrote:
>>> Hi,
>>>
>>> This patch does not allow splitting in case bounds are returned until retutrned bounds are supported.  It also propagates instrumentation marks for generated call and function.
>>>
>>> Bootstrapped and tested on linux-x86_64.
>>>
>>> Thanks,
>>> Ilya
>>> --
>>> gcc/
>>>
>>> 2014-06-03  Ilya Enkovich  <ilya.enkovich@intel.com>
>>>
>>> 	* ipa-split.c: Include tree-chkp.h.
>>> 	(consider_split): Do not split when return bounds.
>>> 	(split_function): Propagate Pointer Bounds Checker
>>> 	instrumentation marks.
>> It's a hack.  There's no reason we can't support this.  So I'll
>> approve on the condition that you do look at removing this
>> limitation in the future.
>>
>> jeff
>>
>
> I did some work for function splitting and now patch cover more cases.  Now returned bounds are supported but it is not allowed to split producers of returned pointer and its bounds.  Is it OK?
>
> Thanks,
> Ilya
> --
> 2014-08-15  Ilya Enkovich  <ilya.enkovich@intel.com>
>
> 	* ipa-split.c: Include tree-chkp.h.
> 	(find_retbnd): New.
> 	(consider_split): Do not split retbnd and retval
> 	producers.
> 	(split_function): Propagate Pointer Bounds Checker
> 	instrumentation marks and handle returned bounds.
I don't think it's sufficient to just look at the SSA_NAME_DEFSTMT and 
verify that it's not in the header.

You could easily have the SSA_NAME_DEF_STMT be a PHI which is in the 
same partition as the RETURN statement.  One of the PHI arguments might 
be fed from a statement in the header, right?

Don't you have to look at the entire set of definitions which directly 
and indirectly feed the return statement and verify that each and every 
one is in the same partition as the return statement?

And if so, that makes me start to think the original hack wasn't so bad 
after all :-)

jeff

Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Ilya Enkovich]

2014-09-03 23:12 GMT+04:00 Jeff Law <law@redhat.com>:
> On 08/18/14 09:55, Ilya Enkovich wrote:
>>
>> On 04 Jun 01:15, Jeff Law wrote:
>>>
>>> On 06/03/14 01:10, Ilya Enkovich wrote:
>>>>
>>>> Hi,
>>>>
>>>> This patch does not allow splitting in case bounds are returned until
>>>> retutrned bounds are supported.  It also propagates instrumentation marks
>>>> for generated call and function.
>>>>
>>>> Bootstrapped and tested on linux-x86_64.
>>>>
>>>> Thanks,
>>>> Ilya
>>>> --
>>>> gcc/
>>>>
>>>> 2014-06-03  Ilya Enkovich  <ilya.enkovich@intel.com>
>>>>
>>>>         * ipa-split.c: Include tree-chkp.h.
>>>>         (consider_split): Do not split when return bounds.
>>>>         (split_function): Propagate Pointer Bounds Checker
>>>>         instrumentation marks.
>>>
>>> It's a hack.  There's no reason we can't support this.  So I'll
>>> approve on the condition that you do look at removing this
>>> limitation in the future.
>>>
>>> jeff
>>>
>>
>> I did some work for function splitting and now patch cover more cases.
>> Now returned bounds are supported but it is not allowed to split producers
>> of returned pointer and its bounds.  Is it OK?
>>
>> Thanks,
>> Ilya
>> --
>> 2014-08-15  Ilya Enkovich  <ilya.enkovich@intel.com>
>>
>>         * ipa-split.c: Include tree-chkp.h.
>>         (find_retbnd): New.
>>         (consider_split): Do not split retbnd and retval
>>         producers.
>>         (split_function): Propagate Pointer Bounds Checker
>>         instrumentation marks and handle returned bounds.
>
> I don't think it's sufficient to just look at the SSA_NAME_DEFSTMT and
> verify that it's not in the header.
>
> You could easily have the SSA_NAME_DEF_STMT be a PHI which is in the same
> partition as the RETURN statement.  One of the PHI arguments might be fed
> from a statement in the header, right?
>
> Don't you have to look at the entire set of definitions which directly and
> indirectly feed the return statement and verify that each and every one is
> in the same partition as the return statement?

A problem I'm trying to avoid is that bounds in return statement are
not taken into account when checking for data dependencies between
parts.  It means we may have a case when return statement with bounds
is put into split part but bounds producer is not.  If
SSA_NAME_DEFSTMT for returned bounds is in the same partition as a
return then I do not think I should care about the rest of definitions
chain because regular split point checks should make sure we have
everything required.

>
> And if so, that makes me start to think the original hack wasn't so bad
> after all :-)

It's always nice to have a backup plan! :)

Ilya

>
> jeff
>

Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Jeff Law]

On 09/15/14 03:51, Ilya Enkovich wrote:
>>> 2014-08-15  Ilya Enkovich  <ilya.enkovich@intel.com>
>>>
>>>          * ipa-split.c: Include tree-chkp.h.
>>>          (find_retbnd): New.
>>>          (consider_split): Do not split retbnd and retval
>>>          producers.
>>>          (split_function): Propagate Pointer Bounds Checker
>>>          instrumentation marks and handle returned bounds.
>>
>> I don't think it's sufficient to just look at the SSA_NAME_DEFSTMT and
>> verify that it's not in the header.
>>
>> You could easily have the SSA_NAME_DEF_STMT be a PHI which is in the same
>> partition as the RETURN statement.  One of the PHI arguments might be fed
>> from a statement in the header, right?
>>
>> Don't you have to look at the entire set of definitions which directly and
>> indirectly feed the return statement and verify that each and every one is
>> in the same partition as the return statement?
>
> A problem I'm trying to avoid is that bounds in return statement are
> not taken into account when checking for data dependencies between
> parts.  It means we may have a case when return statement with bounds
> is put into split part but bounds producer is not.  If
> SSA_NAME_DEFSTMT for returned bounds is in the same partition as a
> return then I do not think I should care about the rest of definitions
> chain because regular split point checks should make sure we have
> everything required.
Is the data dependency in the gimple IL?  If so there shouldn't be 
anything particularly special we need to do.  If not, then how ugly 
would it be to "use" the bounds at the return statement to expose the 
missing dependency?

Not asking you to make that change, just want to make sure that I 
understand the core issue and that if something is missing from a 
dependency standpoint that we consider what it would take to expose the 
missing dependency.

jeff

Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Ilya Enkovich]

2014-09-15 19:39 GMT+04:00 Jeff Law <law@redhat.com>:
> On 09/15/14 03:51, Ilya Enkovich wrote:
>>>>
>>>> 2014-08-15  Ilya Enkovich  <ilya.enkovich@intel.com>
>>>>
>>>>          * ipa-split.c: Include tree-chkp.h.
>>>>          (find_retbnd): New.
>>>>          (consider_split): Do not split retbnd and retval
>>>>          producers.
>>>>          (split_function): Propagate Pointer Bounds Checker
>>>>          instrumentation marks and handle returned bounds.
>>>
>>>
>>> I don't think it's sufficient to just look at the SSA_NAME_DEFSTMT and
>>> verify that it's not in the header.
>>>
>>> You could easily have the SSA_NAME_DEF_STMT be a PHI which is in the same
>>> partition as the RETURN statement.  One of the PHI arguments might be fed
>>> from a statement in the header, right?
>>>
>>> Don't you have to look at the entire set of definitions which directly
>>> and
>>> indirectly feed the return statement and verify that each and every one
>>> is
>>> in the same partition as the return statement?
>>
>>
>> A problem I'm trying to avoid is that bounds in return statement are
>> not taken into account when checking for data dependencies between
>> parts.  It means we may have a case when return statement with bounds
>> is put into split part but bounds producer is not.  If
>> SSA_NAME_DEFSTMT for returned bounds is in the same partition as a
>> return then I do not think I should care about the rest of definitions
>> chain because regular split point checks should make sure we have
>> everything required.
>
> Is the data dependency in the gimple IL?  If so there shouldn't be anything
> particularly special we need to do.  If not, then how ugly would it be to
> "use" the bounds at the return statement to expose the missing dependency?
>
> Not asking you to make that change, just want to make sure that I understand
> the core issue and that if something is missing from a dependency standpoint
> that we consider what it would take to expose the missing dependency.

Gimple IL has required data dependencies to handle returns properly.
But split pass handles return basic block in a special way.  Return
basic block has to have a simple form and is not scanned using stmt
walkers as it is done for all other BBs by visit_bb.  It is assumed
that all dependencies for return BB are PHI args and returned value.
Thus returned bounds are just not taken into account.  That's how I
see the problem.

Ilya

>
> jeff
>

Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Jeff Law]

On 09/15/14 10:20, Ilya Enkovich wrote:
>>>
>>> A problem I'm trying to avoid is that bounds in return statement are
>>> not taken into account when checking for data dependencies between
>>> parts.  It means we may have a case when return statement with bounds
>>> is put into split part but bounds producer is not.  If
>>> SSA_NAME_DEFSTMT for returned bounds is in the same partition as a
>>> return then I do not think I should care about the rest of definitions
>>> chain because regular split point checks should make sure we have
>>> everything required.
>>
>> Is the data dependency in the gimple IL?  If so there shouldn't be anything
>> particularly special we need to do.  If not, then how ugly would it be to
>> "use" the bounds at the return statement to expose the missing dependency?
>>
>> Not asking you to make that change, just want to make sure that I understand
>> the core issue and that if something is missing from a dependency standpoint
>> that we consider what it would take to expose the missing dependency.
>
> Gimple IL has required data dependencies to handle returns properly.
> But split pass handles return basic block in a special way.  Return
> basic block has to have a simple form and is not scanned using stmt
> walkers as it is done for all other BBs by visit_bb.  It is assumed
> that all dependencies for return BB are PHI args and returned value.
> Thus returned bounds are just not taken into account.  That's how I
> see the problem.
I must be misunderstanding something then.  I fundamentally don't see 
how the return bounds are any different here than the return value.  If 
we have exposed the bounds in the IL, then aren't they going to be 
handled just like any other object in the IL?

Maybe you should post the IL for a case where this all matters and walk 
me through the key issues.

jeff

Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Ilya Enkovich]

[-- Attachment #1: Type: text/plain, Size: 3688 bytes --]

2014-09-16 1:08 GMT+04:00 Jeff Law <law@redhat.com>:
> On 09/15/14 10:20, Ilya Enkovich wrote:
>>>>
>>>>
>>>> A problem I'm trying to avoid is that bounds in return statement are
>>>> not taken into account when checking for data dependencies between
>>>> parts.  It means we may have a case when return statement with bounds
>>>> is put into split part but bounds producer is not.  If
>>>> SSA_NAME_DEFSTMT for returned bounds is in the same partition as a
>>>> return then I do not think I should care about the rest of definitions
>>>> chain because regular split point checks should make sure we have
>>>> everything required.
>>>
>>>
>>> Is the data dependency in the gimple IL?  If so there shouldn't be
>>> anything
>>> particularly special we need to do.  If not, then how ugly would it be to
>>> "use" the bounds at the return statement to expose the missing
>>> dependency?
>>>
>>> Not asking you to make that change, just want to make sure that I
>>> understand
>>> the core issue and that if something is missing from a dependency
>>> standpoint
>>> that we consider what it would take to expose the missing dependency.
>>
>>
>> Gimple IL has required data dependencies to handle returns properly.
>> But split pass handles return basic block in a special way.  Return
>> basic block has to have a simple form and is not scanned using stmt
>> walkers as it is done for all other BBs by visit_bb.  It is assumed
>> that all dependencies for return BB are PHI args and returned value.
>> Thus returned bounds are just not taken into account.  That's how I
>> see the problem.
>
> I must be misunderstanding something then.  I fundamentally don't see how
> the return bounds are any different here than the return value.  If we have
> exposed the bounds in the IL, then aren't they going to be handled just like
> any other object in the IL?

They are not handled like any other object in IL because return block
and all statements in it are not handled as all other statements we
put into split part.

Here is a comment from find_return_bb:

/* Return basic block containing RETURN statement.  We allow basic blocks
   of the form:
   <retval> = tmp_var;
   return <retval>
   but return_bb can not be more complex than this.
...
*/

Phi nodes also may present in return_bb.

All blocks going to split part are analyzed by visit_bb function.
Return basic block is not analyzed in the same way but still may be
copied into split part in case return value is defined in it.  There
is a special code in visit_bb to add args of phi statements of
return_bb as uses of split part to have no undefined values in copied
block.  It was enough when those phi args plus return value were only
uses in return_bb.

But now we add returned bounds to GIMPLE_RETURN as a new use and this
new use is ignored.  If split part returns value then return_bb will
be copied into it.  It means I should check returned bounds are
defined there too.  If SSA_NAME_DEF_STMT of returned bounds is in
split part then it is OK.  If SSA_NAME_DEF_STMT of returned bounds is
in return_bb then it is also OK because it means it is a result of PHI
node whose args were added as additional uses for split part earlier
in visit_bb.

At least that is how I think this happens :)

>
> Maybe you should post the IL for a case where this all matters and walk me
> through the key issues.

I attach a dump I got from Chrome compilation with no additional
checks restrictions in split.  Original function returns value defined
by phi node in return_bb and bounds defined in BB2.  Split part
contains BB3, BB4 and BB5 and resulting function part has usage of
returned bounds but no producer for it.

Thanks,
Ilya

>
> jeff

[-- Attachment #2: split.dump --]
[-- Type: application/octet-stream, Size: 15407 bytes --]


void (* TextMapState::pickProc.chkp(#'pointer_bounds_type' not supported by dump_type#<type error>, int, void, ...))(const TextMapState&, const SkScalar*) (struct TextMapState * const this, __bounds_type __chkp_bounds_of_this, int scalarsPerPosition)
{
  __bounds_type __bound_tmp.413;
  SkScalar D.29701;
  __bounds_type __bound_tmp.415;
  SkScalar D.29697;
  __bounds_type __bound_tmp.414;
  SkScalar D.29693;
  __bounds_type __bound_tmp.416;
  SkScalar D.29689;
  __bounds_type __bound_tmp.322;
  unsigned int mtype;
  void (*Proc) (const struct TextMapState &, const SkScalar *) _2;
  const struct SkMatrix & _7;
  unsigned int _10;
  const struct SkMatrix & _12;
  const struct SkMatrix & _16;
  float _20;
  const struct SkMatrix & _21;
  float _24;
  const struct SkMatrix & _25;
  float _28;
  unsigned int _30;
  const struct SkMatrix & * _37;
  const struct SkMatrix & * _39;
  SkScalar * _47;
  SkScalar * _49;
  SkScalar * _55;
  SkScalar * _57;
  SkScalar * _58;
  SkScalar * _60;
  SkScalar * _71;
  SkScalar * _73;
  const SkScalar[9] * _81;
  const SkScalar[9] * _86;
  const SkScalar[9] * _93;
  const SkScalar[9] * _99;
  const SkScalar * _102;
  SkScalar _103;
  const SkScalar * _106;
  const SkScalar * _107;
  SkScalar _108;
  const SkScalar * _111;
  const SkScalar * _112;
  SkScalar _113;
  const SkScalar * _116;
  const SkScalar * _117;
  SkScalar _118;

  <bb 2>:
  __bound_tmp.322_36 = __builtin_ia32_bndmk (0, 0);
  if (scalarsPerPosition_4(D) == 1)
    goto <bb 3>;
  else
    goto <bb 6>;

  <bb 3>:
  _37 = &this_6(D)->fMatrix;
  __builtin_ia32_bndcl (_37, __chkp_bounds_of_this_35(D));
  _39 = &MEM[(void *)this_6(D) + 15B];
  __builtin_ia32_bndcu (_39, __chkp_bounds_of_this_35(D));
  _7 = this_6(D)->fMatrix;
  __bound_tmp.322_41 = __builtin_ia32_bndldx (_37, _7);
  mtype_9 = SkMatrix::getType.chkp (_7, __bound_tmp.322_41);
  _10 = mtype_9 & 12;
  if (_10 != 0)
    goto <bb 6>;
  else
    goto <bb 4>;

  <bb 4>:
  __builtin_ia32_bndcl (_37, __chkp_bounds_of_this_35(D));
  __builtin_ia32_bndcu (_39, __chkp_bounds_of_this_35(D));
  _12 = this_6(D)->fMatrix;
  __bound_tmp.322_46 = __builtin_ia32_bndldx (_37, _12);
  _99 = &_12->fMat;
  __bound_tmp.416_100 = __builtin_ia32_bndmk (_99, 36);
  __bound_tmp.416_101 = __builtin_ia32_bndint (__bound_tmp.416_100, __bound_tmp.322_46);
  __builtin_ia32_bndcl (_99, __bound_tmp.416_101);
  _102 = &MEM[(void *)_12 + 3B];
  __builtin_ia32_bndcu (_102, __bound_tmp.416_101);
  _103 = _12->fMat[0];
  _47 = &this_6(D)->fScaleX;
  __builtin_ia32_bndcl (_47, __chkp_bounds_of_this_35(D));
  _49 = &MEM[(void *)this_6(D) + 31B];
  __builtin_ia32_bndcu (_49, __chkp_bounds_of_this_35(D));
  this_6(D)->fScaleX = _103;
  __builtin_ia32_bndcl (_37, __chkp_bounds_of_this_35(D));
  __builtin_ia32_bndcu (_39, __chkp_bounds_of_this_35(D));
  _16 = this_6(D)->fMatrix;
  __bound_tmp.322_54 = __builtin_ia32_bndldx (_37, _16);
  _81 = &_16->fMat;
  __bound_tmp.414_104 = __builtin_ia32_bndmk (_81, 36);
  __bound_tmp.414_105 = __builtin_ia32_bndint (__bound_tmp.414_104, __bound_tmp.322_54);
  _106 = &_16->fMat[2];
  __builtin_ia32_bndcl (_106, __bound_tmp.414_105);
  _107 = &MEM[(void *)_16 + 11B];
  __builtin_ia32_bndcu (_107, __bound_tmp.414_105);
  _108 = _16->fMat[2];
  _55 = &this_6(D)->fTransX;
  __builtin_ia32_bndcl (_55, __chkp_bounds_of_this_35(D));
  _57 = &MEM[(void *)this_6(D) + 35B];
  __builtin_ia32_bndcu (_57, __chkp_bounds_of_this_35(D));
  this_6(D)->fTransX = _108;
  _58 = &this_6(D)->fY;
  __builtin_ia32_bndcl (_58, __chkp_bounds_of_this_35(D));
  _60 = &MEM[(void *)this_6(D) + 27B];
  __builtin_ia32_bndcu (_60, __chkp_bounds_of_this_35(D));
  _20 = this_6(D)->fY;
  __builtin_ia32_bndcl (_37, __chkp_bounds_of_this_35(D));
  __builtin_ia32_bndcu (_39, __chkp_bounds_of_this_35(D));
  _21 = this_6(D)->fMatrix;
  __bound_tmp.322_65 = __builtin_ia32_bndldx (_37, _21);
  _86 = &_21->fMat;
  __bound_tmp.415_109 = __builtin_ia32_bndmk (_86, 36);
  __bound_tmp.415_110 = __builtin_ia32_bndint (__bound_tmp.415_109, __bound_tmp.322_65);
  _111 = &_21->fMat[4];
  __builtin_ia32_bndcl (_111, __bound_tmp.415_110);
  _112 = &MEM[(void *)_21 + 19B];
  __builtin_ia32_bndcu (_112, __bound_tmp.415_110);
  _113 = _21->fMat[4];
  _24 = _20 * _113;
  __builtin_ia32_bndcl (_37, __chkp_bounds_of_this_35(D));
  __builtin_ia32_bndcu (_39, __chkp_bounds_of_this_35(D));
  _25 = this_6(D)->fMatrix;
  __bound_tmp.322_70 = __builtin_ia32_bndldx (_37, _25);
  _93 = &_25->fMat;
  __bound_tmp.413_114 = __builtin_ia32_bndmk (_93, 36);
  __bound_tmp.413_115 = __builtin_ia32_bndint (__bound_tmp.413_114, __bound_tmp.322_70);
  _116 = &_25->fMat[5];
  __builtin_ia32_bndcl (_116, __bound_tmp.413_115);
  _117 = &MEM[(void *)_25 + 23B];
  __builtin_ia32_bndcu (_117, __bound_tmp.413_115);
  _118 = _25->fMat[5];
  _28 = _24 + _118;
  _71 = &this_6(D)->fTransformedY;
  __builtin_ia32_bndcl (_71, __chkp_bounds_of_this_35(D));
  _73 = &MEM[(void *)this_6(D) + 39B];
  __builtin_ia32_bndcu (_73, __chkp_bounds_of_this_35(D));
  this_6(D)->fTransformedY = _28;
  _30 = mtype_9 & 2;
  if (_30 != 0)
    goto <bb 6>;
  else
    goto <bb 5>;

  <bb 5>:

  <bb 6>:
  # _2 = PHI <MapXProc.chkp(3), MapOnlyTransXProc.chkp(5), MapXYProc.chkp(2), MapOnlyScaleXProc.chkp(4)>
  return _2, __bound_tmp.322_36;

}



Split point at BB 5
  header time: 13260 header size: 69
  split time: 0 split size: 0
  bbs: 5
  SSA names to pass: 
  Refused: split size is smaller than call overhead
found articulation at bb 4
Split point at BB 4
  header time: 9179 header size: 16
  split time: 4081 split size: 53
  bbs: 4, 5
  SSA names to pass: 6, 9, 35, 37, 39
  Refused: need to pass non-param values
found articulation at bb 3
Split point at BB 3
  header time: 5000 header size: 4
  split time: 8260 split size: 65
  bbs: 3, 4, 5
  SSA names to pass: 6, 35
  Accepted!
  New best split point!
found articulation at bb 2
Split point at BB 2
  header time: 2000 header size: 1
  split time: 11260 split size: 68
  bbs: 2, 3, 4, 5
  SSA names to pass: 4, 6, 35
  Refused: incoming frequency is too large.


Splitting function at:
Split point at BB 3
  header time: 5000 header size: 4
  split time: 8260 split size: 65
  bbs: 3, 4, 5
  SSA names to pass: 6, 35
Marking result for renaming : .MEM_3 = PHI <.MEM_78(3), .MEM_29(5), .MEM_5(D)(2), .MEM_29(4)>

;; 1 loops found
;;
;; Loop 0
;;  header 0, latch 1
;;  depth 0, outer -1
;;  nodes: 0 1

Updating SSA:
creating PHI node in block #5 for .MEM
Registering new PHI nodes in block #0
Registering new PHI nodes in block #6
Registering new PHI nodes in block #2
Updating SSA information for statement __builtin_ia32_bndcl (_2, __chkp_bounds_of_this_3(D));
Updating SSA information for statement __builtin_ia32_bndcu (_4, __chkp_bounds_of_this_3(D));
Updating SSA information for statement _5 = this_1(D)->fMatrix;
Updating SSA information for statement mtype_7 = SkMatrix::getType.chkp (_5, __bound_tmp.322_6);
Registering new PHI nodes in block #3
Updating SSA information for statement __builtin_ia32_bndcl (_2, __chkp_bounds_of_this_3(D));
Updating SSA information for statement __builtin_ia32_bndcu (_4, __chkp_bounds_of_this_3(D));
Updating SSA information for statement _9 = this_1(D)->fMatrix;
Updating SSA information for statement __builtin_ia32_bndcl (_11, __bound_tmp.416_13);
Updating SSA information for statement __builtin_ia32_bndcu (_14, __bound_tmp.416_13);
Updating SSA information for statement _15 = _9->fMat[0];
Updating SSA information for statement __builtin_ia32_bndcl (_16, __chkp_bounds_of_this_3(D));
Updating SSA information for statement __builtin_ia32_bndcu (_17, __chkp_bounds_of_this_3(D));
Updating SSA information for statement this_1(D)->fScaleX = _15;
Updating SSA information for statement __builtin_ia32_bndcl (_2, __chkp_bounds_of_this_3(D));
Updating SSA information for statement __builtin_ia32_bndcu (_4, __chkp_bounds_of_this_3(D));
Updating SSA information for statement _18 = this_1(D)->fMatrix;
Updating SSA information for statement __builtin_ia32_bndcl (_23, __bound_tmp.414_22);
Updating SSA information for statement __builtin_ia32_bndcu (_24, __bound_tmp.414_22);
Updating SSA information for statement _25 = _18->fMat[2];
Updating SSA information for statement __builtin_ia32_bndcl (_26, __chkp_bounds_of_this_3(D));
Updating SSA information for statement __builtin_ia32_bndcu (_27, __chkp_bounds_of_this_3(D));
Updating SSA information for statement this_1(D)->fTransX = _25;
Updating SSA information for statement __builtin_ia32_bndcl (_28, __chkp_bounds_of_this_3(D));
Updating SSA information for statement __builtin_ia32_bndcu (_29, __chkp_bounds_of_this_3(D));
Updating SSA information for statement _30 = this_1(D)->fY;
Updating SSA information for statement __builtin_ia32_bndcl (_2, __chkp_bounds_of_this_3(D));
Updating SSA information for statement __builtin_ia32_bndcu (_4, __chkp_bounds_of_this_3(D));
Updating SSA information for statement _31 = this_1(D)->fMatrix;
Updating SSA information for statement __builtin_ia32_bndcl (_36, __bound_tmp.415_35);
Updating SSA information for statement __builtin_ia32_bndcu (_37, __bound_tmp.415_35);
Updating SSA information for statement _38 = _31->fMat[4];
Updating SSA information for statement __builtin_ia32_bndcl (_2, __chkp_bounds_of_this_3(D));
Updating SSA information for statement __builtin_ia32_bndcu (_4, __chkp_bounds_of_this_3(D));
Updating SSA information for statement _40 = this_1(D)->fMatrix;
Updating SSA information for statement __builtin_ia32_bndcl (_45, __bound_tmp.413_44);
Updating SSA information for statement __builtin_ia32_bndcu (_46, __bound_tmp.413_44);
Updating SSA information for statement _47 = _40->fMat[5];
Updating SSA information for statement __builtin_ia32_bndcl (_49, __chkp_bounds_of_this_3(D));
Updating SSA information for statement __builtin_ia32_bndcu (_50, __chkp_bounds_of_this_3(D));
Updating SSA information for statement this_1(D)->fTransformedY = _48;
Registering new PHI nodes in block #4
Registering new PHI nodes in block #5
Updating SSA information for statement return _52, __bound_tmp.322_53;

Symbols to be put in SSA form
{ D.29705 }
Incremental SSA update started at block: 0
Number of blocks in CFG: 7
Number of blocks to update: 6 ( 86%)
Affected blocks: 0 2 3 4 5 6


;; 1 loops found
;;
;; Loop 0
;;  header 0, latch 1
;;  depth 0, outer -1
;;  nodes: 0 1 6 2 3 4 5
;; 6 succs { 2 }
;; 2 succs { 5 3 }
;; 3 succs { 5 4 }
;; 4 succs { 5 }
;; 5 succs { 1 }
void (* TextMapState::pickProc(int))(const TextMapState&, const SkScalar*) (struct TextMapState * const this, __bounds_type __chkp_bounds_of_this)
{
  unsigned int mtype;
  __bounds_type __bound_tmp.322;
  SkScalar D.29717;
  __bounds_type __bound_tmp.416;
  SkScalar D.29715;
  __bounds_type __bound_tmp.414;
  SkScalar D.29713;
  __bounds_type __bound_tmp.415;
  SkScalar D.29711;
  __bounds_type __bound_tmp.413;
  int scalarsPerPosition;
  void (*Proc) (const struct TextMapState &, const SkScalar *) _2;
  const struct SkMatrix & _7;
  unsigned int _10;
  const struct SkMatrix & _12;
  const struct SkMatrix & _16;
  float _20;
  const struct SkMatrix & _21;
  float _24;
  const struct SkMatrix & _25;
  float _28;
  unsigned int _30;
  const struct SkMatrix & * _37;
  const struct SkMatrix & * _39;
  SkScalar * _47;
  SkScalar * _49;
  SkScalar * _55;
  SkScalar * _57;
  SkScalar * _58;
  SkScalar * _60;
  SkScalar * _71;
  SkScalar * _73;
  const SkScalar[9] * _81;
  const SkScalar[9] * _86;
  const SkScalar[9] * _93;
  const SkScalar[9] * _99;
  const SkScalar * _102;
  SkScalar _103;
  const SkScalar * _106;
  const SkScalar * _107;
  SkScalar _108;
  const SkScalar * _111;
  const SkScalar * _112;
  SkScalar _113;
  const SkScalar * _116;
  const SkScalar * _117;
  SkScalar _118;

  <bb 6>:

  <bb 2>:
  _2 = &this_1(D)->fMatrix;
  __builtin_ia32_bndcl (_2, __chkp_bounds_of_this_3(D));
  _4 = &MEM[(void *)this_1(D) + 15B];
  __builtin_ia32_bndcu (_4, __chkp_bounds_of_this_3(D));
  _5 = this_1(D)->fMatrix;
  __bound_tmp.322_6 = __builtin_ia32_bndldx (_2, _5);
  mtype_7 = SkMatrix::getType.chkp (_5, __bound_tmp.322_6);
  _8 = mtype_7 & 12;
  if (_8 != 0)
    goto <bb 5>;
  else
    goto <bb 3>;

  <bb 3>:
  __builtin_ia32_bndcl (_2, __chkp_bounds_of_this_3(D));
  __builtin_ia32_bndcu (_4, __chkp_bounds_of_this_3(D));
  _9 = this_1(D)->fMatrix;
  __bound_tmp.322_10 = __builtin_ia32_bndldx (_2, _9);
  _11 = &_9->fMat;
  __bound_tmp.416_12 = __builtin_ia32_bndmk (_11, 36);
  __bound_tmp.416_13 = __builtin_ia32_bndint (__bound_tmp.416_12, __bound_tmp.322_10);
  __builtin_ia32_bndcl (_11, __bound_tmp.416_13);
  _14 = &MEM[(void *)_9 + 3B];
  __builtin_ia32_bndcu (_14, __bound_tmp.416_13);
  _15 = _9->fMat[0];
  _16 = &this_1(D)->fScaleX;
  __builtin_ia32_bndcl (_16, __chkp_bounds_of_this_3(D));
  _17 = &MEM[(void *)this_1(D) + 31B];
  __builtin_ia32_bndcu (_17, __chkp_bounds_of_this_3(D));
  this_1(D)->fScaleX = _15;
  __builtin_ia32_bndcl (_2, __chkp_bounds_of_this_3(D));
  __builtin_ia32_bndcu (_4, __chkp_bounds_of_this_3(D));
  _18 = this_1(D)->fMatrix;
  __bound_tmp.322_19 = __builtin_ia32_bndldx (_2, _18);
  _20 = &_18->fMat;
  __bound_tmp.414_21 = __builtin_ia32_bndmk (_20, 36);
  __bound_tmp.414_22 = __builtin_ia32_bndint (__bound_tmp.414_21, __bound_tmp.322_19);
  _23 = &_18->fMat[2];
  __builtin_ia32_bndcl (_23, __bound_tmp.414_22);
  _24 = &MEM[(void *)_18 + 11B];
  __builtin_ia32_bndcu (_24, __bound_tmp.414_22);
  _25 = _18->fMat[2];
  _26 = &this_1(D)->fTransX;
  __builtin_ia32_bndcl (_26, __chkp_bounds_of_this_3(D));
  _27 = &MEM[(void *)this_1(D) + 35B];
  __builtin_ia32_bndcu (_27, __chkp_bounds_of_this_3(D));
  this_1(D)->fTransX = _25;
  _28 = &this_1(D)->fY;
  __builtin_ia32_bndcl (_28, __chkp_bounds_of_this_3(D));
  _29 = &MEM[(void *)this_1(D) + 27B];
  __builtin_ia32_bndcu (_29, __chkp_bounds_of_this_3(D));
  _30 = this_1(D)->fY;
  __builtin_ia32_bndcl (_2, __chkp_bounds_of_this_3(D));
  __builtin_ia32_bndcu (_4, __chkp_bounds_of_this_3(D));
  _31 = this_1(D)->fMatrix;
  __bound_tmp.322_32 = __builtin_ia32_bndldx (_2, _31);
  _33 = &_31->fMat;
  __bound_tmp.415_34 = __builtin_ia32_bndmk (_33, 36);
  __bound_tmp.415_35 = __builtin_ia32_bndint (__bound_tmp.415_34, __bound_tmp.322_32);
  _36 = &_31->fMat[4];
  __builtin_ia32_bndcl (_36, __bound_tmp.415_35);
  _37 = &MEM[(void *)_31 + 19B];
  __builtin_ia32_bndcu (_37, __bound_tmp.415_35);
  _38 = _31->fMat[4];
  _39 = _30 * _38;
  __builtin_ia32_bndcl (_2, __chkp_bounds_of_this_3(D));
  __builtin_ia32_bndcu (_4, __chkp_bounds_of_this_3(D));
  _40 = this_1(D)->fMatrix;
  __bound_tmp.322_41 = __builtin_ia32_bndldx (_2, _40);
  _42 = &_40->fMat;
  __bound_tmp.413_43 = __builtin_ia32_bndmk (_42, 36);
  __bound_tmp.413_44 = __builtin_ia32_bndint (__bound_tmp.413_43, __bound_tmp.322_41);
  _45 = &_40->fMat[5];
  __builtin_ia32_bndcl (_45, __bound_tmp.413_44);
  _46 = &MEM[(void *)_40 + 23B];
  __builtin_ia32_bndcu (_46, __bound_tmp.413_44);
  _47 = _40->fMat[5];
  _48 = _39 + _47;
  _49 = &this_1(D)->fTransformedY;
  __builtin_ia32_bndcl (_49, __chkp_bounds_of_this_3(D));
  _50 = &MEM[(void *)this_1(D) + 39B];
  __builtin_ia32_bndcu (_50, __chkp_bounds_of_this_3(D));
  this_1(D)->fTransformedY = _48;
  _51 = mtype_7 & 2;
  if (_51 != 0)
    goto <bb 5>;
  else
    goto <bb 4>;

  <bb 4>:

  <bb 5>:
  # _52 = PHI <MapXProc.chkp(2), MapOnlyScaleXProc.chkp(3), MapOnlyTransXProc.chkp(4)>
  return _52, __bound_tmp.322_53;

}

Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Jeff Law]

On 09/16/14 03:09, Ilya Enkovich wrote:
>>
>> I must be misunderstanding something then.  I fundamentally don't see how
>> the return bounds are any different here than the return value.  If we have
>> exposed the bounds in the IL, then aren't they going to be handled just like
>> any other object in the IL?
>
> They are not handled like any other object in IL because return block
> and all statements in it are not handled as all other statements we
> put into split part.
>
> Here is a comment from find_return_bb:
>
> /* Return basic block containing RETURN statement.  We allow basic blocks
>     of the form:
>     <retval> = tmp_var;
>     return <retval>
>     but return_bb can not be more complex than this.
> ...
> */
>
> Phi nodes also may present in return_bb.
Right.  I've seen this stuff, but it's still not clear to me what the 
real issue is.

The first thing that jumps out when I look at your dump is we don't have 
  a PHI for __bound_tmp.322 in BB6.  Now it may be that we really just 
wanted __bound_tmp.322_36, but that seems wrong as the return value 
varies depending on how we reach BB6 and it seems to me the bounds ought 
to vary in a similar manner.



>
> All blocks going to split part are analyzed by visit_bb function.
> Return basic block is not analyzed in the same way but still may be
> copied into split part in case return value is defined in it.  There
> is a special code in visit_bb to add args of phi statements of
> return_bb as uses of split part to have no undefined values in copied
> block.  It was enough when those phi args plus return value were only
> uses in return_bb.
>
> But now we add returned bounds to GIMPLE_RETURN as a new use and this
> new use is ignored.  If split part returns value then return_bb will
> be copied into it.  It means I should check returned bounds are
> defined there too.  If SSA_NAME_DEF_STMT of returned bounds is in
> split part then it is OK.  If SSA_NAME_DEF_STMT of returned bounds is
> in return_bb then it is also OK because it means it is a result of PHI
> node whose args were added as additional uses for split part earlier
> in visit_bb.
>
> At least that is how I think this happens :)
>
>>
>> Maybe you should post the IL for a case where this all matters and walk me
>> through the key issues.
>
> I attach a dump I got from Chrome compilation with no additional
> checks restrictions in split.  Original function returns value defined
> by phi node in return_bb and bounds defined in BB2.  Split part
> contains BB3, BB4 and BB5 and resulting function part has usage of
> returned bounds but no producer for it.
Right, but my question is whether or not the bounds from BB2 were really 
the correct bounds to be using in the first place!  I would have 
expected a PHI in BB6 to select the bounds based on the path leading to 
BB6, much like we select a different return value.

Jeff

Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Ilya Enkovich]

2014-09-19 23:45 GMT+04:00 Jeff Law <law@redhat.com>:
> On 09/16/14 03:09, Ilya Enkovich wrote:
>>>
>>>
>>> I must be misunderstanding something then.  I fundamentally don't see how
>>> the return bounds are any different here than the return value.  If we
>>> have
>>> exposed the bounds in the IL, then aren't they going to be handled just
>>> like
>>> any other object in the IL?
>>
>>
>> They are not handled like any other object in IL because return block
>> and all statements in it are not handled as all other statements we
>> put into split part.
>>
>> Here is a comment from find_return_bb:
>>
>> /* Return basic block containing RETURN statement.  We allow basic blocks
>>     of the form:
>>     <retval> = tmp_var;
>>     return <retval>
>>     but return_bb can not be more complex than this.
>> ...
>> */
>>
>> Phi nodes also may present in return_bb.
>
> Right.  I've seen this stuff, but it's still not clear to me what the real
> issue is.
>
> The first thing that jumps out when I look at your dump is we don't have  a
> PHI for __bound_tmp.322 in BB6.  Now it may be that we really just wanted
> __bound_tmp.322_36, but that seems wrong as the return value varies
> depending on how we reach BB6 and it seems to me the bounds ought to vary in
> a similar manner.

Bounds don't have to vary for different pointers.  E.g. p and p + 1
always have equal bounds.  In this particular case we have function
pointers and all of them have default bounds.

>
>
>
>>
>> All blocks going to split part are analyzed by visit_bb function.
>> Return basic block is not analyzed in the same way but still may be
>> copied into split part in case return value is defined in it.  There
>> is a special code in visit_bb to add args of phi statements of
>> return_bb as uses of split part to have no undefined values in copied
>> block.  It was enough when those phi args plus return value were only
>> uses in return_bb.
>>
>> But now we add returned bounds to GIMPLE_RETURN as a new use and this
>> new use is ignored.  If split part returns value then return_bb will
>> be copied into it.  It means I should check returned bounds are
>> defined there too.  If SSA_NAME_DEF_STMT of returned bounds is in
>> split part then it is OK.  If SSA_NAME_DEF_STMT of returned bounds is
>> in return_bb then it is also OK because it means it is a result of PHI
>> node whose args were added as additional uses for split part earlier
>> in visit_bb.
>>
>> At least that is how I think this happens :)
>>
>>>
>>> Maybe you should post the IL for a case where this all matters and walk
>>> me
>>> through the key issues.
>>
>>
>> I attach a dump I got from Chrome compilation with no additional
>> checks restrictions in split.  Original function returns value defined
>> by phi node in return_bb and bounds defined in BB2.  Split part
>> contains BB3, BB4 and BB5 and resulting function part has usage of
>> returned bounds but no producer for it.
>
> Right, but my question is whether or not the bounds from BB2 were really the
> correct bounds to be using in the first place!  I would have expected a PHI
> in BB6 to select the bounds based on the path leading to BB6, much like we
> select a different return value.

Consider we have pointer computation and then

return __bnd_init_ptr_bounds (res);

In such case you would never have a PHI node for bounds.  Also do not
forget that we may have no PHI nodes for both return value and return
bounds.  In such case we could also easily fall into undefined value
as in dump.

Thanks,
Ilya

>
> Jeff

Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Jeff Law]

On 09/22/14 00:40, Ilya Enkovich wrote:
>
> Bounds don't have to vary for different pointers.  E.g. p and p + 1
> always have equal bounds.  In this particular case we have function
> pointers and all of them have default bounds.
OK.  It looked a bit odd and I wanted to make sure there wasn't 
something fundamentally wrong.

>>> I attach a dump I got from Chrome compilation with no additional
>>> checks restrictions in split.  Original function returns value defined
>>> by phi node in return_bb and bounds defined in BB2.  Split part
>>> contains BB3, BB4 and BB5 and resulting function part has usage of
>>> returned bounds but no producer for it.
>>
>> Right, but my question is whether or not the bounds from BB2 were really the
>> correct bounds to be using in the first place!  I would have expected a PHI
>> in BB6 to select the bounds based on the path leading to BB6, much like we
>> select a different return value.
>
> Consider we have pointer computation and then
>
> return __bnd_init_ptr_bounds (res);
>
> In such case you would never have a PHI node for bounds.  Also do not
> forget that we may have no PHI nodes for both return value and return
> bounds.  In such case we could also easily fall into undefined value
> as in dump.
This code (visit_bb, find_return_bb, consider_split) is a bit of a mess, 
but I do see what you're trying to do now.  Thanks for being patient 
with my questions.

If I were to look at this at a high level, the core issue seems to me 
that we're really not prepared to handle functions with multiple return 
values.  This shows up in your MPX work, but IIRC there's cases in the 
atomics where we have multiple return values as well.  I wouldn't be 
surprised if there's latent bugs with splitting & atomics lurking to 
bite us one day.

So if I'm reading all this code correctly, given a return block which 
returns a (pointer,bounds) pair, if the bounds are set  by a normal 
statement (ie, not a PHI), then we won't use that block for RETURN_BB. 
  So there's nothing to worry about in that case.  Similarly if the 
bounds are  set by a PHI in the return block, consider_split will reject 
that split point as well.  So really the only case here is when the 
bounds are set in another dominating block.  Right?

I can see how you're using the relevant part of the same test we need 
for the retval.  My gut tells me we want to commonize that test so that 
they don't get out-of-sync.    Specifically, can we pull the code which 
sets split_part_set_retbnd into a little function, then use it for the 
retval here too:

   else if (TREE_CODE (retval) == SSA_NAME)
     current->split_part_set_retval
       = (!SSA_NAME_IS_DEFAULT_DEF (retval)
          && (bitmap_bit_p (current->split_bbs,
                           gimple_bb (SSA_NAME_DEF_STMT (retval))->index)
              || gimple_bb (SSA_NAME_DEF_STMT (retval)) == return_bb));



Iteration through the statements in find_retbnd should start at the end 
of the block and walk backwards.  It probably doesn't matter in practice 
all that much, but might as well be sensible since the GIMPLE_RETURN is 
almost always going to be the last statement in the block.

Similarly for the statement walk in split_function when you want to 
replace retbnd with new one.

It seems like the code to build the bndret call to obtain bounds is 
repeated.  Can you refactor that into its own little function and just 
use that.  It's not a huge amount of code, but it does make things a bit 
easier to follow.

With those changes this will be OK.

Jeff

Re: [PATCH, Pointer Bounds Checker 23/x] Function split

[Ilya Enkovich]

On 23 Sep 09:55, Jeff Law wrote:
> On 09/22/14 00:40, Ilya Enkovich wrote:
> >
> >Bounds don't have to vary for different pointers.  E.g. p and p + 1
> >always have equal bounds.  In this particular case we have function
> >pointers and all of them have default bounds.
> OK.  It looked a bit odd and I wanted to make sure there wasn't
> something fundamentally wrong.
> 
> >>>I attach a dump I got from Chrome compilation with no additional
> >>>checks restrictions in split.  Original function returns value defined
> >>>by phi node in return_bb and bounds defined in BB2.  Split part
> >>>contains BB3, BB4 and BB5 and resulting function part has usage of
> >>>returned bounds but no producer for it.
> >>
> >>Right, but my question is whether or not the bounds from BB2 were really the
> >>correct bounds to be using in the first place!  I would have expected a PHI
> >>in BB6 to select the bounds based on the path leading to BB6, much like we
> >>select a different return value.
> >
> >Consider we have pointer computation and then
> >
> >return __bnd_init_ptr_bounds (res);
> >
> >In such case you would never have a PHI node for bounds.  Also do not
> >forget that we may have no PHI nodes for both return value and return
> >bounds.  In such case we could also easily fall into undefined value
> >as in dump.
> This code (visit_bb, find_return_bb, consider_split) is a bit of a
> mess, but I do see what you're trying to do now.  Thanks for being
> patient with my questions.
> 
> If I were to look at this at a high level, the core issue seems to
> me that we're really not prepared to handle functions with multiple
> return values.  This shows up in your MPX work, but IIRC there's
> cases in the atomics where we have multiple return values as well.
> I wouldn't be surprised if there's latent bugs with splitting &
> atomics lurking to bite us one day.
> 
> So if I'm reading all this code correctly, given a return block
> which returns a (pointer,bounds) pair, if the bounds are set  by a
> normal statement (ie, not a PHI), then we won't use that block for
> RETURN_BB.  So there's nothing to worry about in that case.
> Similarly if the bounds are  set by a PHI in the return block,
> consider_split will reject that split point as well.  So really the
> only case here is when the bounds are set in another dominating
> block.  Right?
> 
> I can see how you're using the relevant part of the same test we
> need for the retval.  My gut tells me we want to commonize that test
> so that they don't get out-of-sync.    Specifically, can we pull the
> code which sets split_part_set_retbnd into a little function, then
> use it for the retval here too:
> 
>   else if (TREE_CODE (retval) == SSA_NAME)
>     current->split_part_set_retval
>       = (!SSA_NAME_IS_DEFAULT_DEF (retval)
>          && (bitmap_bit_p (current->split_bbs,
>                           gimple_bb (SSA_NAME_DEF_STMT (retval))->index)
>              || gimple_bb (SSA_NAME_DEF_STMT (retval)) == return_bb));
> 
> 
> 
> Iteration through the statements in find_retbnd should start at the
> end of the block and walk backwards.  It probably doesn't matter in
> practice all that much, but might as well be sensible since the
> GIMPLE_RETURN is almost always going to be the last statement in the
> block.
> 
> Similarly for the statement walk in split_function when you want to
> replace retbnd with new one.
> 
> It seems like the code to build the bndret call to obtain bounds is
> repeated.  Can you refactor that into its own little function and
> just use that.  It's not a huge amount of code, but it does make
> things a bit easier to follow.
> 
> With those changes this will be OK.
> 
> Jeff
> 
> 

Here is a version with modifications you proposed.  Thanks for review!

Ilya
--
2014-09-25  Ilya Enkovich  <ilya.enkovich@intel.com>

	* ipa-split.c: Include tree-chkp.h.
	(find_retbnd): New.
	(split_part_set_ssa_name_p): New.
	(consider_split): Do not split retbnd and retval
	producers.
	(insert_bndret_call_after): new.
	(split_function): Propagate Pointer Bounds Checker
	instrumentation marks and handle returned bounds.


diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
index 2af3a93..7a1b75e 100644
--- a/gcc/ipa-split.c
+++ b/gcc/ipa-split.c
@@ -110,6 +110,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimple-pretty-print.h"
 #include "ipa-inline.h"
 #include "cfgloop.h"
+#include "tree-chkp.h"
 
 /* Per basic block info.  */
 
@@ -151,6 +152,7 @@ struct split_point best_split_point;
 static bitmap forbidden_dominators;
 
 static tree find_retval (basic_block return_bb);
+static tree find_retbnd (basic_block return_bb);
 
 /* Callback for walk_stmt_load_store_addr_ops.  If T is non-SSA automatic
    variable, check it if it is present in bitmap passed via DATA.  */
@@ -370,6 +372,21 @@ dominated_by_forbidden (basic_block bb)
   return false;
 }
 
+/* For give split point CURRENT and return block RETURN_BB return 1
+   if ssa name VAL is set by split part and 0 otherwise.  */
+static bool
+split_part_set_ssa_name_p (tree val, struct split_point *current,
+			   basic_block return_bb)
+{
+  if (TREE_CODE (val) != SSA_NAME)
+    return false;
+
+  return (!SSA_NAME_IS_DEFAULT_DEF (val)
+	  && (bitmap_bit_p (current->split_bbs,
+			    gimple_bb (SSA_NAME_DEF_STMT (val))->index)
+	      || gimple_bb (SSA_NAME_DEF_STMT (val)) == return_bb));
+}
+
 /* We found an split_point CURRENT.  NON_SSA_VARS is bitmap of all non ssa
    variables used and RETURN_BB is return basic block.
    See if we can split function here.  */
@@ -387,6 +404,7 @@ consider_split (struct split_point *current, bitmap non_ssa_vars,
   unsigned int i;
   int incoming_freq = 0;
   tree retval;
+  tree retbnd;
   bool back_edge = false;
 
   if (dump_file && (dump_flags & TDF_DETAILS))
@@ -575,10 +593,7 @@ consider_split (struct split_point *current, bitmap non_ssa_vars,
        = bitmap_bit_p (non_ssa_vars, DECL_UID (SSA_NAME_VAR (retval)));
   else if (TREE_CODE (retval) == SSA_NAME)
     current->split_part_set_retval
-      = (!SSA_NAME_IS_DEFAULT_DEF (retval)
-	 && (bitmap_bit_p (current->split_bbs,
-			  gimple_bb (SSA_NAME_DEF_STMT (retval))->index)
-	     || gimple_bb (SSA_NAME_DEF_STMT (retval)) == return_bb));
+      = split_part_set_ssa_name_p (retval, current, return_bb);
   else if (TREE_CODE (retval) == PARM_DECL)
     current->split_part_set_retval = false;
   else if (TREE_CODE (retval) == VAR_DECL
@@ -588,6 +603,29 @@ consider_split (struct split_point *current, bitmap non_ssa_vars,
   else
     current->split_part_set_retval = true;
 
+  /* See if retbnd used by return bb is computed by header or split part.  */
+  retbnd = find_retbnd (return_bb);
+  if (retbnd)
+    {
+      bool split_part_set_retbnd
+	= split_part_set_ssa_name_p (retbnd, current, return_bb);
+
+      /* If we have both return value and bounds then keep their definitions
+	 in a single function.  We use SSA names to link returned bounds and
+	 value and therefore do not handle cases when result is passed by
+	 reference (which should not be our case anyway since bounds are
+	 returned for pointers only).  */
+      if ((DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))
+	   && current->split_part_set_retval)
+	  || split_part_set_retbnd != current->split_part_set_retval)
+	{
+	  if (dump_file && (dump_flags & TDF_DETAILS))
+	    fprintf (dump_file,
+		     "  Refused: split point splits return value and bounds\n");
+	  return;
+	}
+    }
+
   /* split_function fixes up at most one PHI non-virtual PHI node in return_bb,
      for the return value.  If there are other PHIs, give up.  */
   if (return_bb != EXIT_BLOCK_PTR_FOR_FN (cfun))
@@ -710,6 +748,18 @@ find_retval (basic_block return_bb)
   return NULL;
 }
 
+/* Given return basic block RETURN_BB, see where return bounds are really
+   stored.  */
+static tree
+find_retbnd (basic_block return_bb)
+{
+  gimple_stmt_iterator bsi;
+  for (bsi = gsi_last_bb (return_bb); !gsi_end_p (bsi); gsi_prev (&bsi))
+    if (gimple_code (gsi_stmt (bsi)) == GIMPLE_RETURN)
+      return gimple_return_retbnd (gsi_stmt (bsi));
+  return NULL;
+}
+
 /* Callback for walk_stmt_load_store_addr_ops.  If T is non-SSA automatic
    variable, mark it as used in bitmap passed via DATA.
    Return true when access to T prevents splitting the function.  */
@@ -1079,6 +1129,19 @@ find_split_points (int overall_time, int overall_size)
   BITMAP_FREE (current.ssa_names_to_pass);
 }
 
+/* Build and insert initialization of returned bounds RETBND
+   for returned value RETVAL.  Statements are inserted after
+   a statement pointed by GSI and GSI is modified to point to
+   the last inserted statement.  */
+
+static void
+insert_bndret_call_after (tree retbnd, tree retval, gimple_stmt_iterator *gsi)
+{
+  tree fndecl = targetm.builtin_chkp_function (BUILT_IN_CHKP_BNDRET);
+  gimple bndret = gimple_build_call (fndecl, 1, retval);
+  gimple_call_set_lhs (bndret, retbnd);
+  gsi_insert_after (gsi, bndret, GSI_CONTINUE_LINKING);
+}
 /* Split function at SPLIT_POINT.  */
 
 static void
@@ -1095,8 +1158,9 @@ split_function (struct split_point *split_point)
   gimple call;
   edge e;
   edge_iterator ei;
-  tree retval = NULL, real_retval = NULL;
+  tree retval = NULL, real_retval = NULL, retbnd = NULL;
   bool split_part_return_p = false;
+  bool with_bounds = chkp_function_instrumented_p (current_function_decl);
   gimple last_stmt = NULL;
   unsigned int i;
   tree arg, ddef;
@@ -1245,6 +1309,12 @@ split_function (struct split_point *split_point)
       DECL_BUILT_IN_CLASS (node->decl) = NOT_BUILT_IN;
       DECL_FUNCTION_CODE (node->decl) = (enum built_in_function) 0;
     }
+
+  /* If the original function is instrumented then it's
+     part is also instrumented.  */
+  if (with_bounds)
+    chkp_function_mark_instrumented (node->decl);
+
   /* If the original function is declared inline, there is no point in issuing
      a warning for the non-inlinable part.  */
   DECL_NO_INLINE_WARNING_P (node->decl) = 1;
@@ -1279,6 +1349,7 @@ split_function (struct split_point *split_point)
 	args_to_pass[i] = arg;
       }
   call = gimple_build_call_vec (node->decl, args_to_pass);
+  gimple_call_set_with_bounds (call, with_bounds);
   gimple_set_block (call, DECL_INITIAL (current_function_decl));
   args_to_pass.release ();
 
@@ -1385,6 +1456,7 @@ split_function (struct split_point *split_point)
       if (return_bb != EXIT_BLOCK_PTR_FOR_FN (cfun))
 	{
 	  real_retval = retval = find_retval (return_bb);
+	  retbnd = find_retbnd (return_bb);
 
 	  if (real_retval && split_point->split_part_set_retval)
 	    {
@@ -1429,6 +1501,21 @@ split_function (struct split_point *split_point)
 			  }
 		      update_stmt (gsi_stmt (bsi));
 		    }
+
+		  /* Replace retbnd with new one.  */
+		  if (retbnd)
+		    {
+		      gimple_stmt_iterator bsi;
+		      for (bsi = gsi_last_bb (return_bb); !gsi_end_p (bsi);
+			   gsi_prev (&bsi))
+			if (gimple_code (gsi_stmt (bsi)) == GIMPLE_RETURN)
+			  {
+			    retbnd = copy_ssa_name (retbnd, call);
+			    gimple_return_set_retbnd (gsi_stmt (bsi), retbnd);
+			    update_stmt (gsi_stmt (bsi));
+			    break;
+			  }
+		    }
 		}
 	      if (DECL_BY_REFERENCE (DECL_RESULT (current_function_decl)))
 		{
@@ -1450,6 +1537,9 @@ split_function (struct split_point *split_point)
 		      gsi_insert_after (&gsi, cpy, GSI_NEW_STMT);
 		      retval = tem;
 		    }
+		  /* Build bndret call to obtain returned bounds.  */
+		  if (retbnd)
+		    insert_bndret_call_after (retbnd, retval, &gsi);
 		  gimple_call_set_lhs (call, retval);
 		  update_stmt (call);
 		}
@@ -1468,6 +1558,10 @@ split_function (struct split_point *split_point)
 	    {
 	      retval = DECL_RESULT (current_function_decl);
 
+	      if (chkp_function_instrumented_p (current_function_decl)
+		  && BOUNDED_P (retval))
+		retbnd = create_tmp_reg (pointer_bounds_type_node, NULL);
+
 	      /* We use temporary register to hold value when aggregate_value_p
 		 is false.  Similarly for DECL_BY_REFERENCE we must avoid extra
 		 copy.  */
@@ -1491,6 +1585,9 @@ split_function (struct split_point *split_point)
 	        gimple_call_set_lhs (call, retval);
 	    }
           gsi_insert_after (&gsi, call, GSI_NEW_STMT);
+	  /* Build bndret call to obtain returned bounds.  */
+	  if (retbnd)
+	    insert_bndret_call_after (retbnd, retval, &gsi);
 	  ret = gimple_build_return (retval);
 	  gsi_insert_after (&gsi, ret, GSI_NEW_STMT);
 	}