Re: [PATCH, Pointer Bounds Checker 14/x] Passes [8/n] Remove useless builtin calls
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 * 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 * 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
Re: [PATCH, Pointer Bounds Checker 14/x] Passes [8/n] Remove useless builtin calls
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
> >
> >* 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
* 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
* 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 000..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 000..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 000..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 000..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
+
Re: [PATCH, Pointer Bounds Checker 14/x] Passes [8/n] Remove useless builtin calls
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 * 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
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
> >
> > * 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
* 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
On Wed, Oct 8, 2014 at 9:08 PM, 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
>
> * 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
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
* 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
[PATCH, Pointer Bounds Checker 14/x] Passes [8/n] Remove useless builtin calls
Hi,
This patch adds removal of user calls to chkp builtins which become useless
after instrumentation.
Thanks,
Ilya
--
2014-10-08 Ilya Enkovich
* 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 ();
