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

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

On Fri, Oct 2, 2009 at 9:10 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Sep 23, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> On Tue, Sep 22, 2009 at 9:32 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
>>> - use something smaller than a DECL structure for DEBUG_EXPR_DECLs. ?We
>>> don't need all the stuff in a DECL, just a uid, a TYPE, a MODE (could be
>>> inferred from the TYPE), and an RTL expansion
>
>> Use an SSA name with a single unique base variable?
>
> If you mean using objects with SSA_NAME tree code, I'd rather not. ?I
> think this would make for a lot of ambiguity and trouble. ?We can't even
> use the same version namespace, for varying version numbers has also
> been known to cause codegen differences.
>
> Now, if you mean some other tree code that uses the same tree_ssa_name
> data structure, that's an idea, but the only field in it that's relevant
> is the version number, and it doesn't have room for the corresponding
> RTX.
>
> Besides, as far as var-tracking is concerned, there are short-term
> benefits to having a DECL, for var-tracking already knows how to deal
> with DECLs. ?Adding support for yet another type of object (tree or rtx)
> would be a bit of a pain, and it could make var-tracking quite slower.
>
> I'm thinking of maybe introducing another data type with a common prefix
> to decl_minimal, or even another base type for decl_minimal. ?But that's
> something for another version of the patch.

Hm, ok.  One reason why I liked the idea of using SSA_NAMEs much is
that it would make it very easy to get rid of old no longer used debug
temps.  How do you address this?

That is, I'd very much like to see some statistics on the number of
debug temps (vs. debug-binds and vs. regular stmts).

>> Uh. ?Please rework that to emit the debug temps at the place of the
>> DEF.
>
> Done. ?Here's the updated patch.

Any reason why in

+      if (TREE_CODE (t) == DEBUG_EXPR_DECL)
+       DECL_UID (t) = --next_debug_decl_uid;

you count backward?  You always print the negative value later...

@@ -368,6 +368,8 @@ gsi_replace (gimple_stmt_iterator *gsi,
   if (stmt == orig_stmt)
     return;

+  insert_debug_temps_for_defs (gsi);
+
   gimple_set_location (stmt, gimple_location (orig_stmt));
   gimple_set_bb (stmt, gsi_bb (*gsi));

@@ -470,6 +472,8 @@ gsi_remove (gimple_stmt_iterator *i, boo
   gimple_seq_node cur, next, prev;
   gimple stmt = gsi_stmt (*i);

+  insert_debug_temps_for_defs (i);
+

for both gsi_replace and gsi_remove you shouldn't really insert
debug-temps.  The replaced/removed stmt may be inserted elsewhere
again and this makes these operations very expensive.  Please leave
the debug handling in the few callers that did this before.

@@ -206,7 +206,7 @@ release_ssa_name (tree var)
       use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (var));

       if (MAY_HAVE_DEBUG_STMTS)
-       propagate_var_def_into_debug_stmts (var, NULL, NULL);
+       insert_debug_temp_for_var_def (NULL, var);

and here I thought that doesn't work because the stmt is already
unlinked?

If you want it more generic I'd say we want to have a release_defs_p
argument to gsi_remove () / gsi_replace () that controls whether
the removed/replaced stmt is going to die.  Just like we have for phi nodes.

On the RTL parts I don't have a strong opinion - though I wonder why
VALUE flows in here so much (I though that was only a cselib thing).

Richard.