Re: [vta, vta4.4] merged with trunk and 4.4 @149247, updated VTA patchset

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

On Mon, Jul 6, 2009 at 4:04 AM, Alexandre Oliva<aoliva@redhat.com> wrote:
> On Jul ?5, 2009, Alexandre Oliva <aoliva@redhat.com> wrote:
>
>> I've just checked in a merge from trunk and gcc-4_4-branch into
>> var-tracking-assignments-branch and var-tracking-assignments-4_4-branch,
>> respectively, both starting from revision 149247.
>
>> -before and -after tags were created, and the merge-point tags were
>> updated to point to the newly-created -trunk and -4_4 merge tags.
>
>> The trunk merge required a few changes:
>
>> - changing rewrite_stmt() to take a stmt_iterator again, rather than a
>> stmt, so that we can insert debug bind stmts
>
>> - adjust mem_loc_descriptor to deal with TLS symbols without using the
>> removed INTERNAL_DW_OP code
>
>> - copying the support for comparing VALUEs from rtl_equal_p_cb to its
>> copy rtl_equal_p
>
>> Here's an updated VTA patchset. ?Is there anything else that needs to be
>> done before we reach a decision on whether it can be merged?

At least there are missing ChangeLog entries.

Comments about the tree parts:

Why do we have the VAR_DEBUG_VALUE tree code?  Isn't
the gimple statement type enough?  When I see

+#define VAR_DEBUG_VALUE_SET_VAR(T, V) (gimple_debug_bind_set_var ((T), (V)))
+#define VAR_DEBUG_VALUE_VAR(T) (gimple_debug_bind_get_var (T))
+#define VAR_DEBUG_VALUE_VALUE(T) (*gimple_debug_bind_get_value_ptr (T))

I am confused anyway.  Didn't I ask that these become (inline)
functions in gimple.[ch]?  Why are the functions named
gimple_debug_bind_* instead of gimple_debug_*?  Do you think
you'll add different kinds of debug statements in the future?
Otherwise please simplify all this by not caring for the subcode
in GIMPLE_DEBUG (you sometimes check is_gimple_debug
and sometimes check gimple_debug_bind_p anyway...).

And we still have

+/* Nonzero if is_gimple_debug() may possibly hold.  */
+#define MAY_HAVE_DEBUG_STMTS    (flag_var_tracking_assignments)

You have my maintainers vote to unconditionally enable
flag_var_tracking_assignments.

+/* Decide whether to emit a VAR_DEBUG_VALUE annotation for VAR.  */
+
+bool
+var_debug_value_for_decl (tree var)
+{

the name of this function suggests that a value is inspected.  But
instead a variable is - how about renaming it to something
less confusing, like vta_track_var_p ().

+void
+adjust_debug_stmts_for_var_def_move (tree var,
+                                    basic_block tobb,
+                                    const gimple_stmt_iterator *togsip)
+{
...
+             if (dominated_by_p (CDI_DOMINATORS, bb, tobb))
+               continue;

you assume dominators are available and correct.

...
+             do
+               {
+                 gsi_prev (&si);
+                 if (gsi_end_p (si))
+                   break;
+               }
+             while (gsi_stmt (si) != stmt);

you are linearly walking stmts.  If you rely on dominators then
why not rely on stmt uids being present and correct?  Btw,
tree-into-ssa.c is not exactly the canonical place for these
kind of functions but instead tree-dfa.c would be IMHO.

+      if (!value && !no_value)
+       {
+         if (SSA_NAME_VALUE (var))
+           value = SSA_NAME_VALUE (var);

whoa ... this is supposed to be completely internal to
jump threading.  Don't use it _please_!

+             if (gimple_code (def_stmt) == GIMPLE_ASSIGN
+                 && TREE_TYPE (gimple_assign_rhs1 (def_stmt))
+                 && gimple_assign_rhs2 (def_stmt)
+                 && TREE_TYPE (gimple_assign_rhs2 (def_stmt)))
+               value = gimple_assign_rhs_to_tree (def_stmt);

err - what is this testing?  Maybe you want
is_gimple_assign (def_stmt)
&& (gimple_assign_rhs_class (def_stmt) == GIMPLE_BINARY_RHS
       || gimple_assign_rhs_class (def_stmt) == GIMPLE_UNARY_RHS))
? Accessing rhs2 unconditionally will certainly ICE ...

Or maybe that !value && !no_value is really dead code - otherwise
it _would_ have ICEd.  Why are you building trees here w/o
checking that tobb is NULL anyway?  You seem to not only
adjust the debug stmts because the definition is moved but you
are adjusting it because it is changed at the same time?  (That
never looks ok to me - debug stmts should never change but
only appear or disappear).

This

+/* Look for any SSA_NAMEs in the VALUE of a VAR_DEBUG_VALUE statement
+   T that have become or are about to become unavailable.
+   AVAILABLE_P, if non-NULL, is used to determine whether the variable
+   is about to become unavailable.  */
+
+void
+check_and_update_debug_stmt (gimple t, bool (*available_p)(tree))

stuff - it looks bogus to check for SSA_NAME_IN_FREE_LIST as
the ssa name might be re-used already.  Certainly calling
check_and_update_debug_stmt from verify_stmt looks very wrong...

Why not simply hook into release_ssa_name?  This way you
save yourself walking and only have to look for all immediate
debug-uses.  IMHO that's the only correct place for correctness
reasons anyway.

-       SET_DEF (def_p, make_ssa_name (var, stmt));
+       SET_DEF (def_p, name = make_ssa_name (var, stmt));
+       if (var_debug_value_for_decl (var))
+         {
+           gimple note = gimple_build_debug_bind (var, name, stmt);
+           gsi_insert_after (&si, note, GSI_SAME_STMT);
+         }

please not.  Split the assignment to a different statement.

Is generating debug stmts at this place necessary?  The information
is still present in the statement itself.

+         /* DOM sometimes threads jumps in such a way that a debug
+            stmt ends up referencing a SSA variable that no longer
+            dominates the debug stmt, but such that all incoming
+            definitions refer to the same definition in an earlier
+            dominator.  We could try to recover that definition
+            somehow, but this will have to do for now.
+
+            Introducing a default definition, which is what
+            maybe_replace_use() would do in such cases, may modify
+            code generation, for the otherwise-unused default
+            definition would never go away, modifying SSA version
+            numbers all over.  */
+         VAR_DEBUG_VALUE_VALUE (stmt) = VAR_DEBUG_VALUE_NOVALUE;
+         update_stmt (stmt);

so, if we unconditionally enable var-tracking-assignments we can
care less?  Or is using a default definition incorrect as far as
debug correctness is concerned?

Index: gcc/tree-ssa-loop-im.c
===================================================================
--- gcc/tree-ssa-loop-im.c.orig 2009-07-05 06:37:06.000000000 -0300
+++ gcc/tree-ssa-loop-im.c      2009-07-05 06:37:09.000000000 -0300
@@ -879,6 +879,7 @@ rewrite_bittest (gimple_stmt_iterator *b
       gimple_cond_set_rhs (use_stmt, build_int_cst_type (TREE_TYPE (name), 0));

       gsi_insert_before (bsi, stmt1, GSI_SAME_STMT);
+      adjust_debug_stmts_for_move (gsi_stmt (*bsi), NULL, NULL);
       gsi_replace (bsi, stmt2, true);

this adjusts the debug stmts as if the stmt would be removed,
correct?  What probably is missing here is a
release_defs on the original statement instead.  Likewise at
the other place in tree-ssa-loop-im.c.  Patches against trunk
are pre-approved if they pass bootstrapping / regtesting.

@@ -6007,7 +6007,7 @@ get_inner_reference (tree exp, HOST_WIDE

   /* Compute cumulative bit-offset for nested component-refs and array-refs,
      and find the ultimate containing object.  */
-  while (1)
+  do
     {
       switch (TREE_CODE (exp))
        {
@@ -6086,6 +6086,7 @@ get_inner_reference (tree exp, HOST_WIDE

       exp = TREE_OPERAND (exp, 0);
     }
+  while (exp);

hmm?  Please revert that change.

@@ -1189,11 +1241,43 @@ separate_decls_in_region (edge entry, ed
                                           name_copies, decl_copies);

...

+  if (has_debug_stmt)
+    for (i = 0; VEC_iterate (basic_block, body, i, bb); i++)
+      if (bb != entry_bb && bb != exit_bb)
+       {
+         for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi);)
...

Please add a comment before that hunk.

+    case GIMPLE_DEBUG:
+      /* ??? Should there be conditional VAR_DEBUG_VALUEs?  */
+      if (gimple_debug_bind_p (gsi_stmt (*gsi)))
+       {
+         VAR_DEBUG_VALUE_VALUE (gsi_stmt (*gsi)) = VAR_DEBUG_VALUE_NOVALUE;
+         update_stmt (gsi_stmt (*gsi));
+       }
+      break;

I wonder if, if you re-set debug-insns this way - aren't you
pessimizing debug information more compared if you would
remove the statement?  That is, a DEBUG = NOVALUE means
the value dies here, correct?

-      /* Create a new deep copy of the statement.  */
-      copy = gimple_copy (stmt);
+      if (gimple_debug_bind_p (stmt))
+       {
+         copy = gimple_build_debug_bind (VAR_DEBUG_VALUE_VAR (stmt),
+                                         VAR_DEBUG_VALUE_VALUE (stmt),
+                                         stmt);
+         VARRAY_PUSH_GENERIC_PTR (id->debug_stmts, copy);

why does gimple_copy not handle GIMPLE_DEBUG statements?
And - *shudder* - new VARRAY use.  Don't.

+  update_stmt (stmt);
+  if (gimple_in_ssa_p (cfun))
+    mark_symbols_for_renaming (stmt);

not necessary to mark anything for renaming.  Btw, why all the
separate machinery for re-mapping and duplicating debug statements?
I suppose because of the arbirtary messy values.

Can you get away w/o that ugly VARRAY by simply special-casing
debug statements in remap_gimple_stmt instead of delaying
all the stuff?  It's really ugly.


+static void
+copy_debug_stmt (gimple stmt, copy_body_data *id)

missing a comment anyway.

+
+static void
+copy_debug_stmts (copy_body_data *id)

Likewise.

+  mark_symbols_for_renaming (note);

no.  I hope debug statements never have VOPs, do they?!


+/* Build a new GIMPLE_DEBUG_BIND statement.
+
+   VAR is bound to VALUE.  */
+
+gimple
+gimple_build_debug_bind_stat (tree var, tree value, gimple stmt MEM_STAT_DECL)
+{

the STMT parameter is undocumented.


+  do
+    gsi_next (i);
+  while (!gsi_end_p (*i) && is_gimple_debug (gsi_stmt (*i)));
+}

add {} braces.

+  do
+    gsi_prev (i);
+  while (!gsi_end_p (*i) && is_gimple_debug (gsi_stmt (*i)));

likewise.

Index: gcc/tree-ssanames.c
===================================================================
--- gcc/tree-ssanames.c.orig    2009-07-05 06:37:06.000000000 -0300
+++ gcc/tree-ssanames.c 2009-07-05 06:37:09.000000000 -0300
@@ -205,6 +205,9 @@ release_ssa_name (tree var)
       int saved_ssa_name_version = SSA_NAME_VERSION (var);
       use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (var));

+      if (MAY_HAVE_DEBUG_STMTS)
+       adjust_debug_stmts_for_var_def_move (var, NULL, NULL);
+

Why adjust for move?  Why not check_and_update_debug_stmt
for each immediate debug-use?

+/* Operand is a use only for purposes of debug information.  */
+#define opf_debug_use   (1 << 3)
+

instead of this at the single place that cares you can check
is_gimple_debug on the stmt.

+    case VAR_DEBUG_VALUE:
+      if (VAR_DEBUG_VALUE_VALUE (stmt) != VAR_DEBUG_VALUE_NOVALUE)
+       get_expr_operands (stmt, &VAR_DEBUG_VALUE_VALUE (stmt),
+                          opf_use | opf_debug_use | opf_no_vops);

opf_no_vops should better not be necessary.  Which means that
the RHS better should contain no memory operands as otherwise
you will generate wrong debug info.

 @deftypefn {GIMPLE function} is_gimple_call (gimple g)
-Return true if the code of g is @code{GIMPLE_CALL}
+Return true if the code of g is @code{GIMPLE_CALL}.
 @end deftypefn

Merge fixes separately please.

Thanks,
Richard.

> I posted this this morning, but the list server didn't let it through
> because of the large collection of patches.
>
> Here they are, in a compressed tarball.
>
> compare-debug-default.patch (ping)
> vta-patchset-cmdline.patch
> vta-patchset-tree-base.patch
> vta-patchset-tree-to-rtl.patch
> vta-patchset-rtl-base.patch
> vta-patchset-tracking.patch
> vta-patchset-tree-adjust.patch
> vta-patchset-rtl-adjust.patch
> vta-patchset-rtl-sched.patch
> vta-patchset-ports.patch
> vta-patchset-guality.patch
> gcc-cp-dwarf-no-omit-default-template-args.patch (ping)
>
>
>
> --
> Alexandre Oliva, freedom fighter ? ?http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/ ? FSF Latin America board member
> Free Software Evangelist ? ? ?Red Hat Brazil Compiler Engineer
>
>