On Fri, Oct 5, 2012 at 2:49 PM, Jakub Jelinek <ja...@redhat.com> wrote:
> 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;
Hmm, indeed.
> 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.
I don't think we want to rely on that ... so just keep the push/pop_cfun.
Thanks,
Richard.
> 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
