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 = PHI<param1(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
