Re: [vta->trunk] VTA merge

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

On Sat, Aug 29, 2009 at 6:04 AM, Alexandre Oliva<aoliva@redhat.com> wrote:
> Hi,
>
> I've been on the road and off-line for nearly the whole week, but I
> managed to complete writing the ChangeLog entry for the VTA merge. ?It's
> the longest ChangeLog I've ever seen, weighting some 36KB, for a 600KB
> patch.
>
> It would be ideal to check it in as quickly as possible, because I'm
> going to be at home and on-line for at least the next two weeks, which
> should be plenty of time to fix any issues that might arise.
>
> So, may I have confirmation of the various approvals I've already got,
> before I addressed the suggestions and requests in them?

        * expr.c (get_inner_reference): Tolerate NULL inner exp.

this should be no longer necessary after the alias-export merge.

        * tree-inline.h: Include varray.h.

hmm.  You are using a VARRAY?  I'm sure I have complained about this
already - please use a VEC.

Your changelog gets garbled starting with config/rs6000/rs6000.c - an
extra leading space on each line.

What gdb version is recommended to be able to benefit from
-fvar-tracking-assignments?  In particular do you in this patchset output
expressions with the new dwarf4 stuff?  Can you expand the documentation
for the switch if that is the case?

How was this patch tested?  In particular which of the primary targets
passed bootstrap and regtesting?

-fvar-tracking-assignments seems to be off by default.  Please consider
turing it on by default (together with -fvar-tracking) if -g is specified - at
least during the remainder of stage1 so we get testing coverage.

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

I don't remember exactly the outcome of the last discussion, but I
at least remember asking you to
s/MAY_HAVE_DEBUG_STMTS/flag_var_tracking_assignments/g

@@ -2526,6 +2526,11 @@ propagate_rhs_into_lhs (gimple stmt, tre
         be successful would be if the use occurs in an ASM_EXPR.  */
       FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
        {
+         /* Leave debug stmts alone.  If we succeed in propagating
+            all non-debug uses, we'll drop the DEF, and propagation
+            into debug stmts will occur then.  */
+         if (gimple_debug_bind_p (use_stmt))
+           continue;

this seems to be the only case (sofar) you check gimple_debug_bind_p
instead of is_gimple_debug.  As there is no difference at the moment
using sometimes one and sometimes the other looks bogus.  Please
eliminate gimple_debug_bind_p in favor of is_gimple_debug.

Note that the GIMPLE_DEBUG_BIND as gf_mask is even more bogus
that the previous additional tree code.  Just use literal zero in the
single place where it is necessary and remove the debug bind
vs. debug stmt confusion.

The has_zero_uses, has_single_use, etc. helpers no longer seem
suitable for inlining.  As a followup please split them into a inlined
head and a tail.  The debug rewrites in terms of the boolean previous
result also makes them hard to follow (as you didn't adjust the comments).

+#if 0
+  /* Should we deal with DECL_DEBUG_EXPR_IS_FROM as well?  */
+  if (DECL_DEBUG_EXPR_IS_FROM (var))
+    return target_for_debug_bind (DECL_DEBUG_EXPR (var));
+#endif

remove #if 0 blocks.

+void
+propagate_defs_into_debug_stmts (gimple def, basic_block tobb,
+                                const gimple_stmt_iterator *togsip)
...
+  FOR_EACH_SSA_DEF_OPERAND (def_p, def, op_iter, SSA_OP_ALL_DEFS)

why do you propagate virtual SSA_NAMEs here?  IMHO you want to
use SSA_OP_DEF here (maybe this mistake is also somewhere else).

+/* Process deferred debug stmts.  In order to give values better odds
+   of being successfully remapped, we delay the processing of debug
+   stmts.  */
+
+static void
+copy_debug_stmts (copy_body_data *id)
+{
+  size_t i, e;
+
+  if (!id->debug_stmts)
+    return;
+
+  for (i = 0, e = VARRAY_ACTIVE_SIZE (id->debug_stmts); i < e; i++)
+    copy_debug_stmt ((gimple) VARRAY_GENERIC_PTR (id->debug_stmts, i), id);
+

"better odds of being successfully remapped"?  Well, ok ... I'll take your word
for it.  As noted previously please use a VEC, not a VARRAY.

+insert_init_debug_bind (copy_body_data *id,
+                       basic_block bb, tree var, tree value,
+                       gimple base_stmt)
+{
...
+  mark_symbols_for_renaming (note);

really?  I can't see why.  As I likely have asked this previously a
comment before
the call looks useful.

+/* Verify a gimple debug statement STMT.
+   Returns true if anything is wrong.  */
+
+static bool
+verify_gimple_debug (gimple stmt ATTRIBUTE_UNUSED)
+{
+  /* ??? FIXME.  */
+  return false;
+}

;)  Please add some very basic stuff, like the lhs should be a plain
var or parm decl and the rhs should be (err, what should it be?).
Even a comment explaining what to check would be ok.

The tree middle-end changes are ok with the above changes
if all the bootstrapping and regtesting works out ok.

You need (if you didn't already get) approval for the C++ parts,
the testsuite parts, the build system changes and the RTL
middle-end pieces.

Please announce the merge 24h in advance and ask the
trunk to be frozen.

Thanks,
Richard.