[PATCH, Pointer Bounds Checker 14/x] Passes [8/n] Remove useless builtin calls

[PATCH, Pointer Bounds Checker 14/x] Passes [8/n] Remove useless builtin calls

[Ilya Enkovich]

Hi,

This patch adds removal of user calls to chkp builtins which become useless after instrumentation.

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

	* tree-chkp.c (chkp_remove_useless_builtins): New.
	(chkp_execute): Remove useless calls to Pointer Bounds
	Checker builtins.


diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index 5443950..b424af8 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -3805,6 +3805,49 @@ chkp_instrument_function (void)
 	}
 }
 
+/* Find init/null/copy_ptr_bounds calls and replace them
+   with assignments.  It should allow better code
+   optimization.  */
+
+static void
+chkp_remove_useless_builtins ()
+{
+  basic_block bb, next;
+  gimple_stmt_iterator gsi;
+
+  bb = ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb;
+  do
+    {
+      next = bb->next_bb;
+      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+        {
+          gimple stmt = gsi_stmt (gsi);
+	  tree fndecl;
+	  enum built_in_function fcode;
+
+	  /* Find builtins returning first arg and replace
+	     them with assignments.  */
+	  if (gimple_code (stmt) == GIMPLE_CALL
+	      && (fndecl = gimple_call_fndecl (stmt))
+	      && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL
+	      && (fcode = DECL_FUNCTION_CODE (fndecl))
+	      && (fcode == BUILT_IN_CHKP_INIT_PTR_BOUNDS
+		  || fcode == BUILT_IN_CHKP_NULL_PTR_BOUNDS
+		  || fcode == BUILT_IN_CHKP_COPY_PTR_BOUNDS
+		  || fcode == BUILT_IN_CHKP_SET_PTR_BOUNDS))
+	    {
+	      tree res = gimple_call_arg (stmt, 0);
+	      if (!update_call_from_tree (&gsi, res))
+		gimplify_and_update_call_from_tree (&gsi, res);
+	      stmt = gsi_stmt (gsi);
+	      update_stmt (stmt);
+	    }
+        }
+      bb = next;
+    }
+  while (bb);
+}
+
 /* Initialize pass.  */
 static void
 chkp_init (void)
@@ -3872,6 +3915,8 @@ chkp_execute (void)
 
   chkp_instrument_function ();
 
+  chkp_remove_useless_builtins ();
+
   chkp_function_mark_instrumented (cfun->decl);
 
   chkp_fix_cfg ();

Re: [PATCH, Pointer Bounds Checker 14/x] Passes [8/n] Remove useless builtin calls

[Jeff Law]

On 10/08/14 13:08, Ilya Enkovich wrote:
> Hi,
>
> This patch adds removal of user calls to chkp builtins which become useless after instrumentation.
>
> Thanks,
> Ilya
> --
> 2014-10-08  Ilya Enkovich  <ilya.enkovich@intel.com>
>
> 	* tree-chkp.c (chkp_remove_useless_builtins): New.
> 	(chkp_execute): Remove useless calls to Pointer Bounds
> 	Checker builtins.
Please put this in the file with the optimizations.  Tests too, which 
may require you to add some dumping bits into this code since you may 
not be able to directly see the behaviour you want in the gimple dumps 
later.

What I'm a bit confused about is it looks like every one of these 
builtin calls you end up optimizing -- why not generate the simple copy 
in the first place and avoid the need for chkp_remove_useless_builtins 
completely?  Clearly I'm missing something here.


>
>
> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
> index 5443950..b424af8 100644
> --- a/gcc/tree-chkp.c
> +++ b/gcc/tree-chkp.c
> @@ -3805,6 +3805,49 @@ chkp_instrument_function (void)
>   	}
>   }
>
> +/* Find init/null/copy_ptr_bounds calls and replace them
> +   with assignments.  It should allow better code
> +   optimization.  */
> +
> +static void
> +chkp_remove_useless_builtins ()
> +{
> +  basic_block bb, next;
> +  gimple_stmt_iterator gsi;
> +
> +  bb = ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb;
> +  do
> +    {
> +      next = bb->next_bb;
> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> +        {
> +          gimple stmt = gsi_stmt (gsi);
> +	  tree fndecl;
> +	  enum built_in_function fcode;
There's some kind of formatting goof on one or more of the preceeding 
lines.  They should be at the same indention level.  Most likely they 
aren't consistent with their use of tabs vs spaces.  A nit, but please 
take care of it.

If we end up keeping this code, then I think it'll be fine with the 
whitespace nit fixed.  I just want to make sure there's a good reason to 
generate these builtins in the first place rather than the more direct 
assignment.

THanks,
Jeff

Re: [PATCH, Pointer Bounds Checker 14/x] Passes [8/n] Remove useless builtin calls

[Richard Biener]

On Wed, Oct 8, 2014 at 9:08 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> Hi,
>
> This patch adds removal of user calls to chkp builtins which become useless after instrumentation.
>
> Thanks,
> Ilya
> --
> 2014-10-08  Ilya Enkovich  <ilya.enkovich@intel.com>
>
>         * tree-chkp.c (chkp_remove_useless_builtins): New.
>         (chkp_execute): Remove useless calls to Pointer Bounds
>         Checker builtins.
>
>
> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
> index 5443950..b424af8 100644
> --- a/gcc/tree-chkp.c
> +++ b/gcc/tree-chkp.c
> @@ -3805,6 +3805,49 @@ chkp_instrument_function (void)
>         }
>  }
>
> +/* Find init/null/copy_ptr_bounds calls and replace them
> +   with assignments.  It should allow better code
> +   optimization.  */
> +
> +static void
> +chkp_remove_useless_builtins ()
> +{
> +  basic_block bb, next;
> +  gimple_stmt_iterator gsi;
> +
> +  bb = ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb;
> +  do
> +    {
> +      next = bb->next_bb;

Please don't use ->next_bb but instead use FOR_EACH_BB_FN (cfun, bb)
instead.

> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> +        {
> +          gimple stmt = gsi_stmt (gsi);
> +         tree fndecl;
> +         enum built_in_function fcode;
> +
> +         /* Find builtins returning first arg and replace
> +            them with assignments.  */
> +         if (gimple_code (stmt) == GIMPLE_CALL
> +             && (fndecl = gimple_call_fndecl (stmt))
> +             && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL
> +             && (fcode = DECL_FUNCTION_CODE (fndecl))
> +             && (fcode == BUILT_IN_CHKP_INIT_PTR_BOUNDS
> +                 || fcode == BUILT_IN_CHKP_NULL_PTR_BOUNDS
> +                 || fcode == BUILT_IN_CHKP_COPY_PTR_BOUNDS
> +                 || fcode == BUILT_IN_CHKP_SET_PTR_BOUNDS))
> +           {
> +             tree res = gimple_call_arg (stmt, 0);
> +             if (!update_call_from_tree (&gsi, res))
> +               gimplify_and_update_call_from_tree (&gsi, res);

update_call_from_tree should always succeed with res being a call argument.

Richard.

> +             stmt = gsi_stmt (gsi);
> +             update_stmt (stmt);
> +           }
> +        }
> +      bb = next;
> +    }
> +  while (bb);
> +}
> +
>  /* Initialize pass.  */
>  static void
>  chkp_init (void)
> @@ -3872,6 +3915,8 @@ chkp_execute (void)
>
>    chkp_instrument_function ();
>
> +  chkp_remove_useless_builtins ();
> +
>    chkp_function_mark_instrumented (cfun->decl);
>
>    chkp_fix_cfg ();

Re: [PATCH, Pointer Bounds Checker 14/x] Passes [8/n] Remove useless builtin calls

[Ilya Enkovich]

On 09 Oct 15:03, Jeff Law wrote:
> On 10/08/14 13:08, Ilya Enkovich wrote:
> >Hi,
> >
> >This patch adds removal of user calls to chkp builtins which become useless after instrumentation.
> >
> >Thanks,
> >Ilya
> >--
> >2014-10-08  Ilya Enkovich  <ilya.enkovich@intel.com>
> >
> >	* tree-chkp.c (chkp_remove_useless_builtins): New.
> >	(chkp_execute): Remove useless calls to Pointer Bounds
> >	Checker builtins.
> Please put this in the file with the optimizations.  Tests too,
> which may require you to add some dumping bits into this code since
> you may not be able to directly see the behaviour you want in the
> gimple dumps later.
> 
> What I'm a bit confused about is it looks like every one of these
> builtin calls you end up optimizing -- why not generate the simple
> copy in the first place and avoid the need for
> chkp_remove_useless_builtins completely?  Clearly I'm missing
> something here.
> 
> 
> >
> >
> >diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
> >index 5443950..b424af8 100644
> >--- a/gcc/tree-chkp.c
> >+++ b/gcc/tree-chkp.c
> >@@ -3805,6 +3805,49 @@ chkp_instrument_function (void)
> >  	}
> >  }
> >
> >+/* Find init/null/copy_ptr_bounds calls and replace them
> >+   with assignments.  It should allow better code
> >+   optimization.  */
> >+
> >+static void
> >+chkp_remove_useless_builtins ()
> >+{
> >+  basic_block bb, next;
> >+  gimple_stmt_iterator gsi;
> >+
> >+  bb = ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb;
> >+  do
> >+    {
> >+      next = bb->next_bb;
> >+      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> >+        {
> >+          gimple stmt = gsi_stmt (gsi);
> >+	  tree fndecl;
> >+	  enum built_in_function fcode;
> There's some kind of formatting goof on one or more of the
> preceeding lines.  They should be at the same indention level.  Most
> likely they aren't consistent with their use of tabs vs spaces.  A
> nit, but please take care of it.
> 
> If we end up keeping this code, then I think it'll be fine with the
> whitespace nit fixed.  I just want to make sure there's a good
> reason to generate these builtins in the first place rather than the
> more direct assignment.
> 
> THanks,
> Jeff

With this code we remove user builtins calls coming from source code.  E.g.:

p2 = (int *)__bnd_init_ptr_bounds (p1);
*p2 = 0;

which means p2 has value of p1 but has default bounds and following store is unchecked.  These calls are important for instrumentation but useless after instrumentation.  I don't think it is a part of checker optimizer because it doesn't optimize instrumentation code.  Also this transformation is trivial enough for O0 and checker optimizer works starting from O2.

Below is a version fixed according to Richard's comments.

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

	* tree-chkp.c (chkp_remove_useless_builtins): New.
	(chkp_execute): Remove useless calls to Pointer Bounds
	Checker builtins.


diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index 170b33b..fde0228 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -3805,6 +3805,44 @@ chkp_instrument_function (void)
 	}
 }
 
+/* Find init/null/copy_ptr_bounds calls and replace them
+   with assignments.  It should allow better code
+   optimization.  */
+
+static void
+chkp_remove_useless_builtins ()
+{
+  basic_block bb, next;
+  gimple_stmt_iterator gsi;
+
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+        {
+	  gimple stmt = gsi_stmt (gsi);
+	  tree fndecl;
+	  enum built_in_function fcode;
+
+	  /* Find builtins returning first arg and replace
+	     them with assignments.  */
+	  if (gimple_code (stmt) == GIMPLE_CALL
+	      && (fndecl = gimple_call_fndecl (stmt))
+	      && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL
+	      && (fcode = DECL_FUNCTION_CODE (fndecl))
+	      && (fcode == BUILT_IN_CHKP_INIT_PTR_BOUNDS
+		  || fcode == BUILT_IN_CHKP_NULL_PTR_BOUNDS
+		  || fcode == BUILT_IN_CHKP_COPY_PTR_BOUNDS
+		  || fcode == BUILT_IN_CHKP_SET_PTR_BOUNDS))
+	    {
+	      tree res = gimple_call_arg (stmt, 0);
+	      update_call_from_tree (&gsi, res);
+	      stmt = gsi_stmt (gsi);
+	      update_stmt (stmt);
+	    }
+        }
+    }
+}
+
 /* Initialize pass.  */
 static void
 chkp_init (void)
@@ -3872,6 +3910,8 @@ chkp_execute (void)
 
   chkp_instrument_function ();
 
+  chkp_remove_useless_builtins ();
+
   chkp_function_mark_instrumented (cfun->decl);
 
   chkp_fix_cfg ();

Re: [PATCH, Pointer Bounds Checker 14/x] Passes [8/n] Remove useless builtin calls

[Jeff Law]

On 10/10/14 08:52, Ilya Enkovich wrote:
>>
>> THanks, Jeff
>
> With this code we remove user builtins calls coming from source code.
> E.g.:
>
> p2 = (int *)__bnd_init_ptr_bounds (p1); *p2 = 0;
>
> which means p2 has value of p1 but has default bounds and following
> store is unchecked.  These calls are important for instrumentation
> but useless after instrumentation.  I don't think it is a part of
> checker optimizer because it doesn't optimize instrumentation code.
> Also this transformation is trivial enough for O0 and checker
> optimizer works starting from O2.
>
> Below is a version fixed according to Richard's comments.
>
> Thanks, Ilya -- 2014-10-10  Ilya Enkovich  <ilya.enkovich@intel.com>
>
> * tree-chkp.c (chkp_remove_useless_builtins): New. (chkp_execute):
> Remove useless calls to Pointer Bounds Checker builtins.
Tests instrumentation are still needed.  With some basic tests and 
instrumentation this will be OK.

I hate to be harping tests, but few developers are going to be familiar 
with the MPX and related infrastructure and those tests are critical to 
helping them know when they break something.

Similarly if the plan is to iterate on improving things, then those 
basic functionality tests will ultimately save time as you can smoke 
test before running larger benchmarks.


jeff

Re: [PATCH, Pointer Bounds Checker 14/x] Passes [8/n] Remove useless builtin calls

[Ilya Enkovich]

On 10 Oct 10:11, Jeff Law wrote:
> On 10/10/14 08:52, Ilya Enkovich wrote:
> >>
> >>THanks, Jeff
> >
> >With this code we remove user builtins calls coming from source code.
> >E.g.:
> >
> >p2 = (int *)__bnd_init_ptr_bounds (p1); *p2 = 0;
> >
> >which means p2 has value of p1 but has default bounds and following
> >store is unchecked.  These calls are important for instrumentation
> >but useless after instrumentation.  I don't think it is a part of
> >checker optimizer because it doesn't optimize instrumentation code.
> >Also this transformation is trivial enough for O0 and checker
> >optimizer works starting from O2.
> >
> >Below is a version fixed according to Richard's comments.
> >
> >Thanks, Ilya -- 2014-10-10  Ilya Enkovich  <ilya.enkovich@intel.com>
> >
> >* tree-chkp.c (chkp_remove_useless_builtins): New. (chkp_execute):
> >Remove useless calls to Pointer Bounds Checker builtins.
> Tests instrumentation are still needed.  With some basic tests and
> instrumentation this will be OK.
> 
> I hate to be harping tests, but few developers are going to be
> familiar with the MPX and related infrastructure and those tests are
> critical to helping them know when they break something.
> 
> Similarly if the plan is to iterate on improving things, then those
> basic functionality tests will ultimately save time as you can smoke
> test before running larger benchmarks.
> 
> 
> jeff

Here is a version with tests added.

Thanks,
Ilya
--
gcc/

2014-10-13  Ilya Enkovich  <ilya.enkovich@intel.com>

	* tree-chkp.c (chkp_remove_useless_builtins): New.
	(chkp_execute): Remove useless calls to Pointer Bounds
	Checker builtins.

gcc/testsuite

2014-10-13  Ilya Enkovich  <ilya.enkovich@intel.com>

	* gcc.target/i386/chkp-builtins-1.c: New.
	* gcc.target/i386/chkp-builtins-2.c: New.
	* gcc.target/i386/chkp-builtins-3.c: New.
	* gcc.target/i386/chkp-builtins-4.c: New.


diff --git a/gcc/testsuite/gcc.target/i386/chkp-builtins-1.c b/gcc/testsuite/gcc.target/i386/chkp-builtins-1.c
new file mode 100644
index 0000000..bcc1198
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/chkp-builtins-1.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -fdump-tree-chkp" } */
+/* { dg-final { scan-tree-dump-not "bnd_init_ptr_bounds" "chkp" } } */
+
+void *
+chkp_test (void *p)
+{
+  return __builtin___bnd_init_ptr_bounds (p);
+}
diff --git a/gcc/testsuite/gcc.target/i386/chkp-builtins-2.c b/gcc/testsuite/gcc.target/i386/chkp-builtins-2.c
new file mode 100644
index 0000000..1f4a244
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/chkp-builtins-2.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -fdump-tree-chkp" } */
+/* { dg-final { scan-tree-dump-not "bnd_copy_ptr_bounds" "chkp" } } */
+
+void *
+chkp_test (void *p, void *q)
+{
+  return __builtin___bnd_copy_ptr_bounds (p, q);
+}
diff --git a/gcc/testsuite/gcc.target/i386/chkp-builtins-3.c b/gcc/testsuite/gcc.target/i386/chkp-builtins-3.c
new file mode 100644
index 0000000..ea54ede
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/chkp-builtins-3.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -fdump-tree-chkp" } */
+/* { dg-final { scan-tree-dump-not "bnd_set_ptr_bounds" "chkp" } } */
+
+void *
+chkp_test (void *p)
+{
+  return __builtin___bnd_set_ptr_bounds (p, 10);
+}
diff --git a/gcc/testsuite/gcc.target/i386/chkp-builtins-4.c b/gcc/testsuite/gcc.target/i386/chkp-builtins-4.c
new file mode 100644
index 0000000..cee780b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/chkp-builtins-4.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -fdump-tree-chkp" } */
+/* { dg-final { scan-tree-dump-not "bnd_null_ptr_bounds" "chkp" } } */
+
+void *
+chkp_test (void *p)
+{
+  return __builtin___bnd_null_ptr_bounds (p);
+}
diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index 9be153a..5957e45 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -3800,6 +3800,44 @@ chkp_instrument_function (void)
 	}
 }
 
+/* Find init/null/copy_ptr_bounds calls and replace them
+   with assignments.  It should allow better code
+   optimization.  */
+
+static void
+chkp_remove_useless_builtins ()
+{
+  basic_block bb;
+  gimple_stmt_iterator gsi;
+
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+        {
+	  gimple stmt = gsi_stmt (gsi);
+	  tree fndecl;
+	  enum built_in_function fcode;
+
+	  /* Find builtins returning first arg and replace
+	     them with assignments.  */
+	  if (gimple_code (stmt) == GIMPLE_CALL
+	      && (fndecl = gimple_call_fndecl (stmt))
+	      && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL
+	      && (fcode = DECL_FUNCTION_CODE (fndecl))
+	      && (fcode == BUILT_IN_CHKP_INIT_PTR_BOUNDS
+		  || fcode == BUILT_IN_CHKP_NULL_PTR_BOUNDS
+		  || fcode == BUILT_IN_CHKP_COPY_PTR_BOUNDS
+		  || fcode == BUILT_IN_CHKP_SET_PTR_BOUNDS))
+	    {
+	      tree res = gimple_call_arg (stmt, 0);
+	      update_call_from_tree (&gsi, res);
+	      stmt = gsi_stmt (gsi);
+	      update_stmt (stmt);
+	    }
+        }
+    }
+}
+
 /* Initialize pass.  */
 static void
 chkp_init (void)
@@ -3867,6 +3905,8 @@ chkp_execute (void)
 
   chkp_instrument_function ();
 
+  chkp_remove_useless_builtins ();
+
   chkp_function_mark_instrumented (cfun->decl);
 
   chkp_fix_cfg ();

Re: [PATCH, Pointer Bounds Checker 14/x] Passes [8/n] Remove useless builtin calls

[Jeff Law]

On 10/13/14 08:07, Ilya Enkovich wrote:
>> Tests instrumentation are still needed.  With some basic tests and
>> instrumentation this will be OK.
>>
>> I hate to be harping tests, but few developers are going to be
>> familiar with the MPX and related infrastructure and those tests are
>> critical to helping them know when they break something.
>>
>> Similarly if the plan is to iterate on improving things, then those
>> basic functionality tests will ultimately save time as you can smoke
>> test before running larger benchmarks.
>>
>>
>> jeff
>
> Here is a version with tests added.
>
> Thanks,
> Ilya
> --
> gcc/
>
> 2014-10-13  Ilya Enkovich  <ilya.enkovich@intel.com>
>
> 	* tree-chkp.c (chkp_remove_useless_builtins): New.
> 	(chkp_execute): Remove useless calls to Pointer Bounds
> 	Checker builtins.
>
> gcc/testsuite
>
> 2014-10-13  Ilya Enkovich  <ilya.enkovich@intel.com>
>
> 	* gcc.target/i386/chkp-builtins-1.c: New.
> 	* gcc.target/i386/chkp-builtins-2.c: New.
> 	* gcc.target/i386/chkp-builtins-3.c: New.
> 	* gcc.target/i386/chkp-builtins-4.c: New.
OK.
Jeff