On Fri, Oct 05, 2012 at 02:20:13PM +0200, Richard Guenther wrote:
> The following could use a comment on what you are doing ...
Will add something.
> > + if (args_to_skip)
> > + for (parm = DECL_ARGUMENTS (current_function_decl), num = 0;
> > + parm; parm = DECL_CHAIN (parm), num++)
> > + if (bitmap_bit_p (args_to_skip, num)
> > + && is_gimple_reg (parm))
> > + {
> > + tree ddecl;
> > + gimple def_temp;
> > +
> > + arg = get_or_create_ssa_default_def (cfun, parm);
> > + if (!MAY_HAVE_DEBUG_STMTS)
> > + continue;
> > + if (debug_args == NULL)
> > + debug_args = decl_debug_args_insert (node->symbol.decl);
> > + ddecl = make_node (DEBUG_EXPR_DECL);
> > + DECL_ARTIFICIAL (ddecl) = 1;
> > + TREE_TYPE (ddecl) = TREE_TYPE (parm);
> > + DECL_MODE (ddecl) = DECL_MODE (parm);
> > + VEC_safe_push (tree, gc, *debug_args, DECL_ORIGIN (parm));
> > + VEC_safe_push (tree, gc, *debug_args, ddecl);
> > + def_temp = gimple_build_debug_bind (ddecl, unshare_expr (arg),
> > + call);
> > + gsi_insert_after (&gsi, def_temp, GSI_NEW_STMT);
> > + }
> > + if (debug_args != NULL)
> > + {
> > + unsigned int i;
> > + tree var, vexpr;
> > + gimple_stmt_iterator cgsi;
> > + gimple def_temp;
> > +
> > + push_cfun (DECL_STRUCT_FUNCTION (node->symbol.decl));
>
> What do you need the push/pop_cfun for? I see ENTRY_BLOCK_PTR
> (easily fixable), but else?
I believe that gsi_insert_before in another function
isn't going to work well.
E.g. update_modified_stmt starts with
if (!ssa_operands_active (cfun))
return;
Or is it ok to use gsi_insert_before_without_update and expect that
when compilation resumes for the modified callee, it will
update all modified stmts? There is not much that needs
to be updated on the stmts, source bind has not setters nor uses
of any SSA_NAMEs or virtual defs/sets, the normal bind has a DEBUG_EXPR_DECL
on the RHS and so is not a use or def of anything as well.
If push_cfun/pop_cfun is not needed, guess the two loops could be merged,
the reason for two of them was just that I didn't want to push_cfun/pop_cfun
for each optimized away parameter.
>
> > + var = BLOCK_VARS (DECL_INITIAL (node->symbol.decl));
> > + i = VEC_length (tree, *debug_args);
> > + cgsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR));
> > + do
> > + {
> > + i -= 2;
> > + while (var != NULL_TREE
> > + && DECL_ABSTRACT_ORIGIN (var)
> > + != VEC_index (tree, *debug_args, i))
> > + var = TREE_CHAIN (var);
> > + if (var == NULL_TREE)
> > + break;
> > + vexpr = make_node (DEBUG_EXPR_DECL);
> > + parm = VEC_index (tree, *debug_args, i);
> > + DECL_ARTIFICIAL (vexpr) = 1;
> > + TREE_TYPE (vexpr) = TREE_TYPE (parm);
> > + DECL_MODE (vexpr) = DECL_MODE (parm);
> > + def_temp = gimple_build_debug_source_bind (vexpr, parm,
> > + NULL);
> > + gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT);
> > + def_temp = gimple_build_debug_bind (var, vexpr, NULL);
> > + gsi_insert_before (&cgsi, def_temp, GSI_SAME_STMT);
> > + }
> > + while (i);
> > + pop_cfun ();
> > + }
> > +
> > + t = VEC_index (tree, *debug_args, i + 1);
> > + gimple_debug_source_bind_set_value (stmt, t);
> > + stmt->gsbase.subcode = GIMPLE_DEBUG_BIND;
>
> That looks fishy ... shouldn't it be at least the other way around?
>
> stmt->gsbase.subcode = GIMPLE_DEBUG_BIND;
> gimple_debug_bind_set_value (stmt, t);
It surely can. I was considering whether to create a new wrapper
in gimple.h for the subcode change of a debug stmt, but if there are
no further places needing this (don't see them right now), that might be
overkill.
> Otherwise this looks ok.
Thanks.
Jakub
