Re: [PR debug/41343] introduce debug expr temps

[Richard Guenther <richard dot guenther at gmail dot com>]

On Mon, Oct 12, 2009 at 8:11 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Oct 11, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> + ?/* First of all, check whether there are debug stmts that reference
>> + ? ? this variable and, if there are, decide whether we can */
>
>> missing '. '
>
> Uhh... ?That, and more. ?I clearly didn't finish it.
>
>> you didn't address my review comment here. ?All this checking is
>> completely unnecessary. ?GIMPLE_BINARY_RHS and GIMPLE_UNARY_RHS
>> can only ever have reg or constant operands, only the GIMPLE_SINGLE_RHS
>> case is interesting.
>
> Aah, I thought you meant a different simplification.
>
>> Btw, I think I miss a
>
>> ? if (is_gimple_min_invariant (value)
>> ? ? ? || is_gimple_reg (value))
>> ? ? usetemp = false;
>
>> before these checks.
>
> Indeed. ?The simplified logic in the revised patch sort of obviates (or
> implies) it.
>
>> - ? ? ? ? SET_USE (use_p, unshare_expr (value));
>> + ? ? ? ? /* unshare_expr is not needed here. ?vexpr is either a
>> + ? ? ? ? ? ?SINGLE_RHS, that can be safely shared, some other RHS
>> + ? ? ? ? ? ?that was unshared when we found it had a single debug
>> + ? ? ? ? ? ?use, or a DEBUG_EXPR_DECL, that can be safely
>> + ? ? ? ? ? ?shared. ?*/
>> + ? ? ? ? SET_USE (use_p, value);
>
>> that's not true. ?Value could be &a.b.c
>
> Oh! ?I thought that would have been unary, rather than single. ?Wow.
> Learning new things every day ;-)
>
>> If you want to save memory you have to tighten is_gimple_min_invariant
>> (value) to CONSTANT_CLASS_P (value).
>
> *nod* ?Fixed.
>
>> I think it's better to write this as
>
>> ? ?gcc_assert (!gimple_has_lhs (orig_stmt)
>> ? ? ? ? ? ? ? ? ? ? ? || gimple_get_lhs (orig_stmt) == gimple_get_lhs (stmt));
>
> Agreed.
>
> Here's the revised insert_debug_temp_for_var_def(), currently under
> testing, followed by the revised patch.

Ah, nice to see this in once piece ...

>
> /* Insert a DEBUG BIND stmt before the DEF of VAR if VAR is referenced
> ? by other DEBUG stmts, and replace uses of the DEF with the
> ? newly-created debug temp. ?*/
>
> void
> insert_debug_temp_for_var_def (gimple_stmt_iterator *gsi, tree var)
> {
> ?imm_use_iterator imm_iter;
> ?use_operand_p use_p;
> ?gimple stmt;
> ?gimple def_stmt = NULL;
> ?bool no_value = false;
> ?int usecount = 0;
> ?tree value = NULL;
>
> ?if (!MAY_HAVE_DEBUG_STMTS)
> ? ?return;
>
> ?/* First of all, check whether there are debug stmts that reference
> ? ? this variable and, if there are, decide whether we should use a
> ? ? debug temp. ?*/
> ?FOR_EACH_IMM_USE_FAST (use_p, imm_iter, var)
> ? ?{
> ? ? ?stmt = USE_STMT (use_p);
>
> ? ? ?if (!gimple_debug_bind_p (stmt))
> ? ? ? ?continue;
>
> ? ? ?if (usecount++)
> ? ? ? ?break;
>
> ? ? ?if (gimple_debug_bind_get_value (stmt) != var)
> ? ? ? ?{
> ? ? ? ? ?/* Count this as an additional use, so as to make sure we
> ? ? ? ? ? ? use a temp unless VAR's definition has a SINGLE_RHS that
> ? ? ? ? ? ? can be shared. ?*/
> ? ? ? ? ?usecount++;
> ? ? ? ? ?break;
> ? ? ? ?}
> ? ?}
>
> ?if (!usecount)
> ? ?return;
>
> ?FOR_EACH_IMM_USE_STMT (stmt, imm_iter, var)
> ? ?{
> ? ? ?if (!gimple_debug_bind_p (stmt))
> ? ? ? ?continue;
>
> ? ? ?/* Here we compute (lazily) the value assigned to VAR, but we
> ? ? ? ? remember if we tried before and failed, so that we don't try
> ? ? ? ? again. ?*/

I believe with first looping over all uses you can now move the
following block out of this loop making things a load easier to read.

> ? ? ?if (!value && !no_value)
> ? ? ? ?{
> ? ? ? ? ?if (gsi)
> ? ? ? ? ? ?def_stmt = gsi_stmt (*gsi);
> ? ? ? ? ?else
> ? ? ? ? ? ?def_stmt = SSA_NAME_DEF_STMT (var);
>
> ? ? ? ? ?/* If we didn't get an insertion point, and the stmt has
> ? ? ? ? ? ? already been removed, we won't be able to insert the
> ? ? ? ? ? ? debug bind stmt, so we'll have to drop debug
> ? ? ? ? ? ? information. ?*/
> ? ? ? ? ?if (is_gimple_assign (def_stmt))
> ? ? ? ? ? ?{
> ? ? ? ? ? ? ?if (!dom_info_available_p (CDI_DOMINATORS))
> ? ? ? ? ? ? ? ?{
> ? ? ? ? ? ? ? ? ?struct walk_stmt_info wi;
>
> ? ? ? ? ? ? ? ? ?memset (&wi, 0, sizeof (wi));
>
> ? ? ? ? ? ? ? ? ?/* When removing blocks without following reverse
> ? ? ? ? ? ? ? ? ? ? dominance order, we may sometimes encounter SSA_NAMEs
> ? ? ? ? ? ? ? ? ? ? that have already been released, referenced in other
> ? ? ? ? ? ? ? ? ? ? SSA_DEFs that we're about to release. ?Consider:
>
> ? ? ? ? ? ? ? ? ? ? <bb X>:
> ? ? ? ? ? ? ? ? ? ? v_1 = foo;
>
> ? ? ? ? ? ? ? ? ? ? <bb Y>:
> ? ? ? ? ? ? ? ? ? ? w_2 = v_1 + bar;
> ? ? ? ? ? ? ? ? ? ? # DEBUG w => w_2
>
> ? ? ? ? ? ? ? ? ? ? If we deleted BB X first, propagating the value of
> ? ? ? ? ? ? ? ? ? ? w_2 won't do us any good. ?It's too late to recover
> ? ? ? ? ? ? ? ? ? ? their original definition of v_1: when it was
> ? ? ? ? ? ? ? ? ? ? deleted, it was only referenced in other DEFs, it
> ? ? ? ? ? ? ? ? ? ? couldn't possibly know it should have been retained,
> ? ? ? ? ? ? ? ? ? ? and propagating every single DEF just in case it
> ? ? ? ? ? ? ? ? ? ? might have to be propagated into a DEBUG STMT would
> ? ? ? ? ? ? ? ? ? ? probably be too wasteful.
>
> ? ? ? ? ? ? ? ? ? ? When dominator information is not readily
> ? ? ? ? ? ? ? ? ? ? available, we check for and accept some loss of
> ? ? ? ? ? ? ? ? ? ? debug information. ?But if it is available,
> ? ? ? ? ? ? ? ? ? ? there's no excuse for us to remove blocks in the
> ? ? ? ? ? ? ? ? ? ? wrong order, so we don't even check for dead SSA
> ? ? ? ? ? ? ? ? ? ? NAMEs. ?SSA verification shall catch any
> ? ? ? ? ? ? ? ? ? ? errors. ?*/
> ? ? ? ? ? ? ? ? ?if ((!gsi && !gimple_bb (def_stmt))
> ? ? ? ? ? ? ? ? ? ? ?|| !walk_gimple_op (def_stmt, find_released_ssa_name,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&wi))
> ? ? ? ? ? ? ? ? ? ?no_value = true;
> ? ? ? ? ? ? ? ?}
>
> ? ? ? ? ? ? ?if (!no_value)
> ? ? ? ? ? ? ? ?value = gimple_assign_rhs_to_tree (def_stmt);
> ? ? ? ? ? ?}
>
> ? ? ? ? ?if (!value)
> ? ? ? ? ? ?no_value = true;
> ? ? ? ? ?else
> ? ? ? ? ? ?{
> ? ? ? ? ? ? ?bool no_temp;
>
> ? ? ? ? ? ? ?/* If there's a single use, and VAR is the entire
> ? ? ? ? ? ? ? ? expression (usecount would have been incremented
> ? ? ? ? ? ? ? ? again if not), then we can propagate VALUE into this
> ? ? ? ? ? ? ? ? single use, avoiding the temp.
>
> ? ? ? ? ? ? ? ? We can also avoid using a temp if VALUE can be shared
> ? ? ? ? ? ? ? ? and propagated into all uses, without generating
> ? ? ? ? ? ? ? ? expressions that wouldn't be valid gimple RHSs.
>
> ? ? ? ? ? ? ? ? Other cases that would require unsharing or
> ? ? ? ? ? ? ? ? non-gimple RHSs are deferred to a debug temp,
> ? ? ? ? ? ? ? ? although we could avoid temps at the expense of
> ? ? ? ? ? ? ? ? duplication of expressions. ?*/
>
> ? ? ? ? ? ? ?if (usecount == 1)
> ? ? ? ? ? ? ? ?no_temp = (!gimple_assign_single_p (def_stmt)
> ? ? ? ? ? ? ? ? ? ? ? ? ? || is_gimple_min_invariant (value)
> ? ? ? ? ? ? ? ? ? ? ? ? ? || is_gimple_reg (value));
> ? ? ? ? ? ? ?else
> ? ? ? ? ? ? ? ?{
> ? ? ? ? ? ? ? ? ?no_temp = (CONSTANT_CLASS_P (value)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? || is_gimple_reg (value));
> ? ? ? ? ? ? ? ? ?gcc_assert (!no_temp || unshare_expr (value) == value);

Eh, drop this assert please ...

> ? ? ? ? ? ? ? ?}
>
> ? ? ? ? ? ? ?if (no_temp)
> ? ? ? ? ? ? ? ?value = unshare_expr (value);
> ? ? ? ? ? ? ?else
> ? ? ? ? ? ? ? ?{
> ? ? ? ? ? ? ? ? ?gimple def_temp;
> ? ? ? ? ? ? ? ? ?tree vexpr = make_node (DEBUG_EXPR_DECL);
>
> ? ? ? ? ? ? ? ? ?def_temp = gimple_build_debug_bind (vexpr,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unshare_expr (value),
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?def_stmt);
>
> ? ? ? ? ? ? ? ? ?DECL_ARTIFICIAL (vexpr) = 1;
> ? ? ? ? ? ? ? ? ?TREE_TYPE (vexpr) = TREE_TYPE (value);
> ? ? ? ? ? ? ? ? ?if (DECL_P (value))
> ? ? ? ? ? ? ? ? ? ?DECL_MODE (vexpr) = DECL_MODE (value);
> ? ? ? ? ? ? ? ? ?else
> ? ? ? ? ? ? ? ? ? ?DECL_MODE (vexpr) = TYPE_MODE (TREE_TYPE (value));
>
> ? ? ? ? ? ? ? ? ?if (gsi)
> ? ? ? ? ? ? ? ? ? ?gsi_insert_before (gsi, def_temp, GSI_SAME_STMT);
> ? ? ? ? ? ? ? ? ?else
> ? ? ? ? ? ? ? ? ? ?{
> ? ? ? ? ? ? ? ? ? ? ?gimple_stmt_iterator ngsi = gsi_for_stmt (def_stmt);
> ? ? ? ? ? ? ? ? ? ? ?gsi_insert_before (&ngsi, def_temp, GSI_SAME_STMT);
> ? ? ? ? ? ? ? ? ? ?}
>
> ? ? ? ? ? ? ? ? ?value = vexpr;
> ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ?}
> ? ? ? ?}

In fact you can move all of the above out of the loop IMHO.

> ? ? ?if (no_value)
> ? ? ? ?gimple_debug_bind_reset_value (stmt);
> ? ? ?else
> ? ? ? ?FOR_EACH_IMM_USE_ON_STMT (use_p, imm_iter)
> ? ? ? ? ?/* unshare_expr is not needed here. ?vexpr is either a
> ? ? ? ? ? ? SINGLE_RHS, that can be safely shared, some other RHS
> ? ? ? ? ? ? that was unshared when we found it had a single debug
> ? ? ? ? ? ? use, or a DEBUG_EXPR_DECL, that can be safely
> ? ? ? ? ? ? shared. ?*/
> ? ? ? ? ?SET_USE (use_p, value);
>
> ? ? ?update_stmt (stmt);
> ? ?}
> }

Ok with that change.

Richard.