Re: Minimize downward code motion during reassociation

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

On Fri, Dec 7, 2012 at 9:01 PM, Easwaran Raman <eraman@google.com> wrote:
> It seems I need to reset the debug uses of a statement before moving
> the statement itself. The attached patch starts from the leaf to root
> of the tree to be reassociated and places them at the point where
> their dependences will be met after reassociation. This bootstraps and
> I am running the tests. Ok if there are no test failures?

+  if ((dep_bb != insert_bb
+       && !dominated_by_p (CDI_DOMINATORS, insert_bb, dep_bb))
+      || (dep_bb == insert_bb
+          && gimple_uid (insert_stmt) < gimple_uid (dep_stmt)))
+    {
+      insert_stmt = dep_stmt;
+    }

superfluous {}

+  /* If there are any debug uses of LHS, reset them.  */
+  FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
+    {
+      if (is_gimple_debug (use_stmt))
+        {
+          gimple_debug_bind_reset_value (use_stmt);

that's only needed for debug stmts that are not dominated by insert_point, no?

+  /* Statements marked for throw can not be in the middle of a basic block. So
+     we can not insert a statement (not marked for throw) immediately
after.  */
+  else if (stmt_can_throw_internal (insert_point))
+    {

use stmt_ends_bb_p (insert_point) instead

+      edge succ_edge = find_fallthru_edge (insert_bb->succs);
+      insert_bb = succ_edge->dest;
+      /* Insert STMT at the beginning of the successor basic block.  */
+      gsi_insert = gsi_after_labels (insert_bb);
+      gsi_move_before (&gsistmt, &gsi_insert);

are you sure this is a proper insert location?  How would you even arrive in
this situation given find_insert_point dominance checks?

Otherwise the patch looks ok - can you add one or two testcases please?

Thanks,
Richard.

> Thanks,
> Easwaran
>
> 2012-12-07   Easwaran Raman  <eraman@google.com>
> * tree-ssa-reassoc.c(find_insert_point): New function.
> (insert_stmt_after): Likewise.
> (get_def_stmt): Likewise.
> (ensure_ops_are_available): Likewise.
> (rewrite_expr_tree): Do not move statements beyond what is
> necessary. Remove call to swap_ops_for_binary_stmt...
> (reassociate_bb): ... and move it here.
> (build_and_add_sum): Assign UIDs for new statements.
> (linearize_expr): Likewise.
> (do_reassoc): Renumber gimple statement UIDs.
>
>
>
> On Thu, Dec 6, 2012 at 1:10 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Tue, Nov 6, 2012 at 1:54 AM, Easwaran Raman <eraman@google.com> wrote:
>>> I am unable to figure out the right way to handle the debug
>>> statements. What I tried was to find debug statements that use the SSA
>>> name defined by the statement I moved (using SSA_NAME_IMM_USE_NODE)
>>> and then moved them as well at the right place. Thus, if I have to
>>> move t1 = a + b down (after the definition of 'd'), I also moved all
>>> debug statements that use t1 after the new position of t1. That still
>>> caused use-before-def problems in ssa_verify. I noticed that the debug
>>> statements got modified behind the scenes causing these issues. Any
>>> hints on what is the right way to handle the debug statements would be
>>> very helpful.
>>
>> I think you cannot (and should not) move debug statements.  Instead you
>> have to invalidate them.  Otherwise you'll introduce confusion as debug
>> info cannot handle overlapping live ranges.
>>
>> But maybe Alex can clarify.
>>
>> Richard.