Re: [PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion
On Tue, Nov 19, 2013 at 8:02 PM, Jeff Law l...@redhat.com wrote:
On 11/18/13 14:03, Ilya Enkovich wrote:
2013/11/19 Jeff Law l...@redhat.com:
On 11/18/13 12:16, Ilya Enkovich wrote:
With current recursion elimination we will have:
test (int *param1)
{
bb1:
bb2:
_7 = PHIparam1(D) (bb1), _6 (bb2)
bounds2 = __builtin_arg_bounds (_7) -- WRONG
I wouldn't say it's wrong. It's conservatively correct if properly
implemented. Why precisely do you consider this wrong? If your code
can't
handle it, it really should be fixed.
It is wrong because __builtin_arg_bounds is used to get bounds of
input parameter and PHI node here is not an input parameter.
OK, now we're getting somewhere. So we've got this odd little function
which only works on parameters. I can't help but feel this is a bit of
mis-design coming back to haunt us and I wonder if we're going to see other
instances of this kind of problem.
Right.
There's something just wrong with the semantics of __builtin_arg_bounds, but
I'm having trouble putting my finger on it.
Well, the issue is that it accepts any pointer argument as input.
I originally thought it may just get an integer constant argument - the
argument position. It really seems to me that we have
__builtin_arg_boundsN (), but to avoid having N builtins we specify N
via an argument to the function. But as it may not change we should
use something that cannot change - like a constant. A constant
that identifies a parameter is one that gives its position for example.
Note that any restrictions you impose on what is valid for GIMPLE
should be verified in tree-cfg.c:verify_gimple_* - in the
__builtin_arg_bounds call case in verify_gimple_call.
Richard.
Correctl
handling of __builtin_arg_bounds in this 'wrong' example requires
adding it a PHI node semantics based on it's arg. For me it seems
more complex then generating a regular PHI node for bounds and makes
GIMPLE less readable.
But presumably you have code to merge bound information already, right? You
need that for PHI nodes. Can't you use that to walk up the use-def chains
and build the bounds information?
Or if you want to fix the tailcall stuff, you can start down that direction.
I don't think that'll fix the nagging concerns about the overall semantics
of builtin_arg_bounds, but it might be enough for you to go forward.
jeff
Re: [PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion
2013/11/20 Richard Biener richard.guent...@gmail.com:
On Tue, Nov 19, 2013 at 8:02 PM, Jeff Law l...@redhat.com wrote:
On 11/18/13 14:03, Ilya Enkovich wrote:
2013/11/19 Jeff Law l...@redhat.com:
On 11/18/13 12:16, Ilya Enkovich wrote:
With current recursion elimination we will have:
test (int *param1)
{
bb1:
bb2:
_7 = PHIparam1(D) (bb1), _6 (bb2)
bounds2 = __builtin_arg_bounds (_7) -- WRONG
I wouldn't say it's wrong. It's conservatively correct if properly
implemented. Why precisely do you consider this wrong? If your code
can't
handle it, it really should be fixed.
It is wrong because __builtin_arg_bounds is used to get bounds of
input parameter and PHI node here is not an input parameter.
OK, now we're getting somewhere. So we've got this odd little function
which only works on parameters. I can't help but feel this is a bit of
mis-design coming back to haunt us and I wonder if we're going to see other
instances of this kind of problem.
Right.
There's something just wrong with the semantics of __builtin_arg_bounds, but
I'm having trouble putting my finger on it.
Well, the issue is that it accepts any pointer argument as input.
I originally thought it may just get an integer constant argument - the
argument position. It really seems to me that we have
__builtin_arg_boundsN (), but to avoid having N builtins we specify N
via an argument to the function. But as it may not change we should
use something that cannot change - like a constant. A constant
that identifies a parameter is one that gives its position for example.
I have tried to pass constant for arg_bounds. It still requires
additional modification of optimization passes (including tail
recursion, param propagation, inline etc.) But having constant as an
argument you really cannot detect errors. E.g. if you inline and do
not fix inlined arg_bounds calls correctly, you may not get ICE but
get wrong program which uses wrong bounds and produces false bounds
violations.
Note that any restrictions you impose on what is valid for GIMPLE
should be verified in tree-cfg.c:verify_gimple_* - in the
__builtin_arg_bounds call case in verify_gimple_call.
Will add it.
Thanks,
Ilya
Richard.
Correctl
handling of __builtin_arg_bounds in this 'wrong' example requires
adding it a PHI node semantics based on it's arg. For me it seems
more complex then generating a regular PHI node for bounds and makes
GIMPLE less readable.
But presumably you have code to merge bound information already, right? You
need that for PHI nodes. Can't you use that to walk up the use-def chains
and build the bounds information?
Or if you want to fix the tailcall stuff, you can start down that direction.
I don't think that'll fix the nagging concerns about the overall semantics
of builtin_arg_bounds, but it might be enough for you to go forward.
jeff
Re: [PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion
On Wed, Nov 20, 2013 at 11:41 AM, Ilya Enkovich enkovich@gmail.com wrote:
2013/11/20 Richard Biener richard.guent...@gmail.com:
On Tue, Nov 19, 2013 at 8:02 PM, Jeff Law l...@redhat.com wrote:
On 11/18/13 14:03, Ilya Enkovich wrote:
2013/11/19 Jeff Law l...@redhat.com:
On 11/18/13 12:16, Ilya Enkovich wrote:
With current recursion elimination we will have:
test (int *param1)
{
bb1:
bb2:
_7 = PHIparam1(D) (bb1), _6 (bb2)
bounds2 = __builtin_arg_bounds (_7) -- WRONG
I wouldn't say it's wrong. It's conservatively correct if properly
implemented. Why precisely do you consider this wrong? If your code
can't
handle it, it really should be fixed.
It is wrong because __builtin_arg_bounds is used to get bounds of
input parameter and PHI node here is not an input parameter.
OK, now we're getting somewhere. So we've got this odd little function
which only works on parameters. I can't help but feel this is a bit of
mis-design coming back to haunt us and I wonder if we're going to see other
instances of this kind of problem.
Right.
There's something just wrong with the semantics of __builtin_arg_bounds, but
I'm having trouble putting my finger on it.
Well, the issue is that it accepts any pointer argument as input.
I originally thought it may just get an integer constant argument - the
argument position. It really seems to me that we have
__builtin_arg_boundsN (), but to avoid having N builtins we specify N
via an argument to the function. But as it may not change we should
use something that cannot change - like a constant. A constant
that identifies a parameter is one that gives its position for example.
I have tried to pass constant for arg_bounds. It still requires
additional modification of optimization passes (including tail
recursion, param propagation, inline etc.) But having constant as an
argument you really cannot detect errors. E.g. if you inline and do
not fix inlined arg_bounds calls correctly, you may not get ICE but
get wrong program which uses wrong bounds and produces false bounds
violations.
Yeah, that's why I refrained from suggesting this earlier ;) You lose
the connection between bounds for argument X and function entry
value for argument X.
Richard.
Note that any restrictions you impose on what is valid for GIMPLE
should be verified in tree-cfg.c:verify_gimple_* - in the
__builtin_arg_bounds call case in verify_gimple_call.
Will add it.
Thanks,
Ilya
Richard.
Correctl
handling of __builtin_arg_bounds in this 'wrong' example requires
adding it a PHI node semantics based on it's arg. For me it seems
more complex then generating a regular PHI node for bounds and makes
GIMPLE less readable.
But presumably you have code to merge bound information already, right? You
need that for PHI nodes. Can't you use that to walk up the use-def chains
and build the bounds information?
Or if you want to fix the tailcall stuff, you can start down that direction.
I don't think that'll fix the nagging concerns about the overall semantics
of builtin_arg_bounds, but it might be enough for you to go forward.
jeff
Re: [PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion
On 11/19/13 15:41, Ilya Enkovich wrote:
On 19 Nov 12:02, Jeff Law wrote:
On 11/18/13 14:03, Ilya Enkovich wrote:
2013/11/19 Jeff Law l...@redhat.com:
On 11/18/13 12:16, Ilya Enkovich wrote:
With current recursion elimination we will have:
test (int *param1)
{
bb1:
bb2:
_7 = PHIparam1(D) (bb1), _6 (bb2)
bounds2 = __builtin_arg_bounds (_7) -- WRONG
I wouldn't say it's wrong. It's conservatively correct if properly
implemented. Why precisely do you consider this wrong? If your code can't
handle it, it really should be fixed.
It is wrong because __builtin_arg_bounds is used to get bounds of
input parameter and PHI node here is not an input parameter.
OK, now we're getting somewhere. So we've got this odd little
function which only works on parameters. I can't help but feel
this is a bit of mis-design coming back to haunt us and I wonder if
we're going to see other instances of this kind of problem.
There's something just wrong with the semantics of
__builtin_arg_bounds, but I'm having trouble putting my finger on
it.
Correctl
handling of __builtin_arg_bounds in this 'wrong' example requires
adding it a PHI node semantics based on it's arg. For me it seems
more complex then generating a regular PHI node for bounds and makes
GIMPLE less readable.
But presumably you have code to merge bound information already,
right? You need that for PHI nodes. Can't you use that to walk up
the use-def chains and build the bounds information?
Or if you want to fix the tailcall stuff, you can start down that
direction. I don't think that'll fix the nagging concerns about the
overall semantics of builtin_arg_bounds, but it might be enough for
you to go forward.
jeff
Here are my thoughs on what tail recursion elimination fix should look like.
This patch moves all BUILT_IN_CHKP_ARG_BND calls before the recursion target bb
and creates PHI nodes for bounds.
I tried it on a simple test case only:
int *bar (int *);
void foo (int *p)
{
int *p1 = bar (p);
if (p1)
foo (p1);
}
Tail recursion results in a GIMPLE:
foo (int * p)
{
unnamed type __bound_tmp.0;
int * p1;
void * _8;
void * _10;
bb 2:
__bound_tmp.0_12 = __builtin_ia32_arg_bnd (p_11(D)); [return slot
optimization]
bb 3:
# p_3(ab) = PHI p_11(D)(2), _10(4)
# __bound_tmp.0_7 = PHI __bound_tmp.0_12(2), __bound_tmp.0_9(4)
_8 = __builtin___chkp_bind_bounds (p_3(ab), __bound_tmp.0_7);
p1_5 = bar (_8);
__bound_tmp.0_9 = __builtin_ia32_bndret (p1_5); [return slot optimization]
if (p1_5 != 0B)
goto bb 4;
else
goto bb 5;
bb 4:
_10 = __builtin___chkp_bind_bounds (p1_5, __bound_tmp.0_9);
goto bb 3;
bb 5:
return;
}
Here __bound_tmp.0_7 was created to get input bounds __bound_tmp.0_12 and
__bound_tmp.0_12 passed to the tail call.
If you feel it is good to have such support now instead of disabling tail
recursion elimination - I'll continue working on this patch.
Thanks,
Ilya
--
2013-11-19 Ilya Enkovich ilya.enkov...@intel.com
* tree-tailcall.c (eliminate_tail_call): Fill PHI nodes
created for bounds wit bounds passed to the removed call.
(tree_optimize_tail_calls_1): Move all BUILT_IN_CHKP_ARG_BND
calls before entry point of recursive call. Add PHI nodes
for bounds.
Seems like you're on the right path. The key points being you need to
have the tail recursion reentry point after __builtin_arg_bounds.
first = single_succ (ENTRY_BLOCK_PTR);
+ /* Skip bb created for arg_bnd calls. */
+ for (param = DECL_ARGUMENTS (current_function_decl);
+ param;
+ param = DECL_CHAIN (param))
+if (ssa_default_def (cfun, param))
+ {
+ tree bnd = chkp_argbnd_by_val (ssa_default_def (cfun, param));
+ if (bnd gimple_bb (SSA_NAME_DEF_STMT (bnd)) == first)
+ {
+ arg_bnd_bb = first;
+ first = single_succ (first);
+ break;
+ }
+ }
So in the comment you should say *why* you're skipping over the arg_bnd
calls. ANyone reading this thread knows why, but there should be a
comment in the code itself.
Something like
/* The tail recursion reentry point must be after all the
__builtin_arg_bounds calls to prevent it from being called with anything
but arguments for the current function. */
Jeff
Re: [PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion
On 11/18/13 14:03, Ilya Enkovich wrote:
2013/11/19 Jeff Law l...@redhat.com:
On 11/18/13 12:16, Ilya Enkovich wrote:
With current recursion elimination we will have:
test (int *param1)
{
bb1:
bb2:
_7 = PHIparam1(D) (bb1), _6 (bb2)
bounds2 = __builtin_arg_bounds (_7) -- WRONG
I wouldn't say it's wrong. It's conservatively correct if properly
implemented. Why precisely do you consider this wrong? If your code can't
handle it, it really should be fixed.
It is wrong because __builtin_arg_bounds is used to get bounds of
input parameter and PHI node here is not an input parameter.
OK, now we're getting somewhere. So we've got this odd little function
which only works on parameters. I can't help but feel this is a bit of
mis-design coming back to haunt us and I wonder if we're going to see
other instances of this kind of problem.
There's something just wrong with the semantics of __builtin_arg_bounds,
but I'm having trouble putting my finger on it.
Correctl
handling of __builtin_arg_bounds in this 'wrong' example requires
adding it a PHI node semantics based on it's arg. For me it seems
more complex then generating a regular PHI node for bounds and makes
GIMPLE less readable.
But presumably you have code to merge bound information already, right?
You need that for PHI nodes. Can't you use that to walk up the
use-def chains and build the bounds information?
Or if you want to fix the tailcall stuff, you can start down that
direction. I don't think that'll fix the nagging concerns about the
overall semantics of builtin_arg_bounds, but it might be enough for you
to go forward.
jeff
Re: [PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion
On 19 Nov 12:02, Jeff Law wrote:
On 11/18/13 14:03, Ilya Enkovich wrote:
2013/11/19 Jeff Law l...@redhat.com:
On 11/18/13 12:16, Ilya Enkovich wrote:
With current recursion elimination we will have:
test (int *param1)
{
bb1:
bb2:
_7 = PHIparam1(D) (bb1), _6 (bb2)
bounds2 = __builtin_arg_bounds (_7) -- WRONG
I wouldn't say it's wrong. It's conservatively correct if properly
implemented. Why precisely do you consider this wrong? If your code can't
handle it, it really should be fixed.
It is wrong because __builtin_arg_bounds is used to get bounds of
input parameter and PHI node here is not an input parameter.
OK, now we're getting somewhere. So we've got this odd little
function which only works on parameters. I can't help but feel
this is a bit of mis-design coming back to haunt us and I wonder if
we're going to see other instances of this kind of problem.
There's something just wrong with the semantics of
__builtin_arg_bounds, but I'm having trouble putting my finger on
it.
Correctl
handling of __builtin_arg_bounds in this 'wrong' example requires
adding it a PHI node semantics based on it's arg. For me it seems
more complex then generating a regular PHI node for bounds and makes
GIMPLE less readable.
But presumably you have code to merge bound information already,
right? You need that for PHI nodes. Can't you use that to walk up
the use-def chains and build the bounds information?
Or if you want to fix the tailcall stuff, you can start down that
direction. I don't think that'll fix the nagging concerns about the
overall semantics of builtin_arg_bounds, but it might be enough for
you to go forward.
jeff
Here are my thoughs on what tail recursion elimination fix should look like.
This patch moves all BUILT_IN_CHKP_ARG_BND calls before the recursion target bb
and creates PHI nodes for bounds.
I tried it on a simple test case only:
int *bar (int *);
void foo (int *p)
{
int *p1 = bar (p);
if (p1)
foo (p1);
}
Tail recursion results in a GIMPLE:
foo (int * p)
{
unnamed type __bound_tmp.0;
int * p1;
void * _8;
void * _10;
bb 2:
__bound_tmp.0_12 = __builtin_ia32_arg_bnd (p_11(D)); [return slot
optimization]
bb 3:
# p_3(ab) = PHI p_11(D)(2), _10(4)
# __bound_tmp.0_7 = PHI __bound_tmp.0_12(2), __bound_tmp.0_9(4)
_8 = __builtin___chkp_bind_bounds (p_3(ab), __bound_tmp.0_7);
p1_5 = bar (_8);
__bound_tmp.0_9 = __builtin_ia32_bndret (p1_5); [return slot optimization]
if (p1_5 != 0B)
goto bb 4;
else
goto bb 5;
bb 4:
_10 = __builtin___chkp_bind_bounds (p1_5, __bound_tmp.0_9);
goto bb 3;
bb 5:
return;
}
Here __bound_tmp.0_7 was created to get input bounds __bound_tmp.0_12 and
__bound_tmp.0_12 passed to the tail call.
If you feel it is good to have such support now instead of disabling tail
recursion elimination - I'll continue working on this patch.
Thanks,
Ilya
--
2013-11-19 Ilya Enkovich ilya.enkov...@intel.com
* tree-tailcall.c (eliminate_tail_call): Fill PHI nodes
created for bounds wit bounds passed to the removed call.
(tree_optimize_tail_calls_1): Move all BUILT_IN_CHKP_ARG_BND
calls before entry point of recursive call. Add PHI nodes
for bounds.
diff --git a/gcc/tree-tailcall.c b/gcc/tree-tailcall.c
index 59845950..4e04199 100644
--- a/gcc/tree-tailcall.c
+++ b/gcc/tree-tailcall.c
@@ -814,11 +800,12 @@ eliminate_tail_call (struct tailcall *t)
gimple stmt, call;
tree arg;
size_t idx;
- basic_block bb, first;
+ basic_block bb, first, arg_bnd_bb;
edge e;
gimple phi;
gimple_stmt_iterator gsi;
gimple orig_stmt;
+ tree default_bounds;
stmt = orig_stmt = gsi_stmt (t-call_gsi);
bb = gsi_bb (t-call_gsi);
@@ -834,6 +821,20 @@ eliminate_tail_call (struct tailcall *t)
gcc_assert (is_gimple_call (stmt));
first = single_succ (ENTRY_BLOCK_PTR);
+ /* Skip bb created for arg_bnd calls. */
+ for (param = DECL_ARGUMENTS (current_function_decl);
+ param;
+ param = DECL_CHAIN (param))
+if (ssa_default_def (cfun, param))
+ {
+ tree bnd = chkp_argbnd_by_val (ssa_default_def (cfun, param));
+ if (bnd gimple_bb (SSA_NAME_DEF_STMT (bnd)) == first)
+ {
+ arg_bnd_bb = first;
+ first = single_succ (first);
+ break;
+ }
+ }
/* Remove the code after call_gsi that will become unreachable. The
possibly unreachable code in other blocks is removed later in
@@ -881,6 +882,39 @@ eliminate_tail_call (struct tailcall *t)
add_phi_arg (phi, arg, e, gimple_location (stmt));
gsi_next (gsi);
+
+ /* Fill bounds PHI node with bounds if it follows PHI for arg. */
+ if (!gsi_end_p (gsi)
+ POINTER_BOUNDS_P (gimple_phi_result (gsi_stmt (gsi
+ {
+ tree bounds = NULL;
+
+ phi = gsi_stmt (gsi);
+ if (TREE_CODE (arg) == SSA_NAME)
+ bounds =
[PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion
Hi,
Here is a patch to disable tail recursion transformation when bounds are passed
by call. The reason is BUILT_IN_CHKP_ARG_BND which should always get default
SSA_NAME of PARM_DECL as an argument.
Thanks,
Ilya
--
2013-11-15 Ilya Enkovich ilya.enkov...@intel.com
* tree-tailcall.c: Include tree-chkp.h.
(suitable_for_tail_opt_p): Disable tail
recursion for instrumented functions with bounded args.
diff --git a/gcc/tree-tailcall.c b/gcc/tree-tailcall.c
index 185bf16..59845950 100644
--- a/gcc/tree-tailcall.c
+++ b/gcc/tree-tailcall.c
@@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If not see
#include cfgloop.h
#include common/common-target.h
#include ipa-utils.h
+#include tree-chkp.h
/* The file implements the tail recursion elimination. It is also used to
analyze the tail calls in general, passing the results to the rtl level
@@ -141,6 +142,20 @@ suitable_for_tail_opt_p (void)
if (cfun-stdarg)
return false;
+ /* Tail recursion elimination may cause arg_bnd builtins to be called
+ not for PARM_DECL which is not allowed now. Avoid optimization
+ in such cases for now. */
+ if (chkp_function_instrumented_p (current_function_decl))
+{
+ tree param;
+
+ for (param = DECL_ARGUMENTS (current_function_decl);
+ param;
+ param = DECL_CHAIN (param))
+ if (BOUNDED_P (param))
+ return false;
+}
+
return true;
}
/* Returns false when the function is not suitable for tail call optimization
Re: [PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion
On 11/18/13 03:37, Ilya Enkovich wrote: Hi, Here is a patch to disable tail recursion transformation when bounds are passed by call. The reason is BUILT_IN_CHKP_ARG_BND which should always get default SSA_NAME of PARM_DECL as an argument. Thanks, Ilya -- 2013-11-15 Ilya Enkovich ilya.enkov...@intel.com * tree-tailcall.c: Include tree-chkp.h. (suitable_for_tail_opt_p): Disable tail recursion for instrumented functions with bounded args. This sounds wrong. If the builtins are called with a PARAM_DECL rather than an SSA_NAME, can't they make worst case assumptions about the bounds? In general if we find ourselves disabling an optimizer to make the bounds checker happy, we've got some explaining to do. jeff
Re: [PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion
2013/11/18 Jeff Law l...@redhat.com: On 11/18/13 03:37, Ilya Enkovich wrote: Hi, Here is a patch to disable tail recursion transformation when bounds are passed by call. The reason is BUILT_IN_CHKP_ARG_BND which should always get default SSA_NAME of PARM_DECL as an argument. Thanks, Ilya -- 2013-11-15 Ilya Enkovich ilya.enkov...@intel.com * tree-tailcall.c: Include tree-chkp.h. (suitable_for_tail_opt_p): Disable tail recursion for instrumented functions with bounded args. This sounds wrong. If the builtins are called with a PARAM_DECL rather than an SSA_NAME, can't they make worst case assumptions about the bounds? In general if we find ourselves disabling an optimizer to make the bounds checker happy, we've got some explaining to do. In SSA we are not allowed to have PARAM_DECL as call arg. The problem in this optimization is that when we replace a tail call with jump, we replace default SSA name of input parameter with PHI node holding taking original param and call's arg. This PHI node then is used in BUILT_IN_CHKP_ARG_BND, but is should not. Optimizer has to be taught to analizy bind_bounds of replaced call and generate PHI nodes for bounds. In general for not important optimizations I resolve the stability issue with instrumented code and will enable optimizations later. For obviously important optimizations, like inline, support is initially added. Ilya jeff
Re: [PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion
On 11/18/13 11:10, Ilya Enkovich wrote: In SSA we are not allowed to have PARAM_DECL as call arg. Right. The problem in this optimization is that when we replace a tail call with jump, we replace default SSA name of input parameter with PHI node holding taking original param and call's arg. ? This would be an indication of a problem at the call site. When the recursive call is transformed into a jump we should be taking arguments from the call site and using those as values in a PHI node at the target of the newly created edge See eliminate_tail_call. In general for not important optimizations I resolve the stability issue with instrumented code and will enable optimizations later. For obviously important optimizations, like inline, support is initially added. To me, disabling this appears like you're got something fundamentally wrong with your implementation. Jeff
Re: [PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion
2013/11/18 Jeff Law l...@redhat.com:
On 11/18/13 11:10, Ilya Enkovich wrote:
In SSA we are not allowed to have PARAM_DECL as call arg.
Right.
The problem
in this optimization is that when we replace a tail call with jump, we
replace default SSA name of input parameter with PHI node holding
taking original param and call's arg.
? This would be an indication of a problem at the call site.
Consider following example with tail recursion:
test (int *param1)
{
bounds2 = __builtin_arg_bounds (param1(D))
_3 = __builtin_bind_bounds(param1(D), bounds2);
_4 = some_other_call (_3);
bounds5 = __builtin_ret_bounds(_4);
_6 = __builtin_bind_bounds (_4, bounds5);
test(_6);
}
With current recursion elimination we will have:
test (int *param1)
{
bb1:
bb2:
_7 = PHIparam1(D) (bb1), _6 (bb2)
bounds2 = __builtin_arg_bounds (_7) -- WRONG
_3 = __builtin_bind_bounds(_7, bounds2);
_4 = some_other_call (_3);
bounds5 = __builtin_ret_bounds(_4);
_6 = __builtin_bind_bounds (_4, bounds5);
goto bb2
}
What is required is:
test (int *param1)
{
bb1:
bounds2 = __builtin_arg_bounds (param1(D))
bb2:
_7 = PHIparam1(D) (bb1), _6 (bb2)
_8 = PHIbounds2 (bb1), bounds5 (bb2)
_3 = __builtin_bind_bounds(_7, _8);
_4 = some_other_call (_3);
bounds5 = __builtin_ret_bounds(_4);
_6 = __builtin_bind_bounds (_4, bounds5);
goto bb2
}
So, optimizer has to handle _builtin_arg_bounds in a special way and
search for __builtin_bind_bounds for replaced calls to generate proper
PHI nodes for bounds.
When the recursive call is transformed into a jump we should be taking
arguments from the call site and using those as values in a PHI node at the
target of the newly created edge
See eliminate_tail_call.
In general for not important optimizations I resolve the stability
issue with instrumented code and will enable optimizations later. For
obviously important optimizations, like inline, support is initially
added.
To me, disabling this appears like you're got something fundamentally wrong
with your implementation.
It would be wrong if there is no possibility to enable tail recursion
elimination without changes in instrumentation. But such possibility
exists.
Ilya
Jeff
Re: [PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion
On 11/18/13 12:16, Ilya Enkovich wrote:
With current recursion elimination we will have:
test (int *param1)
{
bb1:
bb2:
_7 = PHIparam1(D) (bb1), _6 (bb2)
bounds2 = __builtin_arg_bounds (_7) -- WRONG
I wouldn't say it's wrong. It's conservatively correct if properly
implemented. Why precisely do you consider this wrong? If your code
can't handle it, it really should be fixed.
Jeff
Re: [PATCH, MPX, 2/X] Pointers Checker [16/25] Tail recursion
2013/11/19 Jeff Law l...@redhat.com:
On 11/18/13 12:16, Ilya Enkovich wrote:
With current recursion elimination we will have:
test (int *param1)
{
bb1:
bb2:
_7 = PHIparam1(D) (bb1), _6 (bb2)
bounds2 = __builtin_arg_bounds (_7) -- WRONG
I wouldn't say it's wrong. It's conservatively correct if properly
implemented. Why precisely do you consider this wrong? If your code can't
handle it, it really should be fixed.
It is wrong because __builtin_arg_bounds is used to get bounds of
input parameter and PHI node here is not an input parameter. Correctl
handling of __builtin_arg_bounds in this 'wrong' example requires
adding it a PHI node semantics based on it's arg. For me it seems
more complex then generating a regular PHI node for bounds and makes
GIMPLE less readable.
Ilya
Jeff
