This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fix PR middle-end/57393


On Mon, Sep 23, 2013 at 5:56 PM, Easwaran Raman <eraman@google.com> wrote:
> Ping.
>
>
> On Mon, Sep 16, 2013 at 4:42 PM, Easwaran Raman <eraman@google.com> wrote:
>> There are two separate root causes for the problem reported in PR
>> 57393. This patch attempts to fix both.
>>
>> First is due to newly created stmts that have the default UID of 0
>> which are compared with statements with valid UIDs leading to broken
>> dependences. As discussed in an earlier thread, I check the UIDs
>> before using them and ensure stmts have a valid UID. In the worst
>> case, this reassigns UIDs for the entire BB containing the stmts in
>> question.
>>
>> The second is due to debug stmts being out of sync with the IR after
>> reassociation. I think the right fix is to create debug temps before
>> actual reassociation to decouple them from the SSA variables involved
>> in reassociation.
>>
>> This bootstraps in x86_64 and I am running the tests. Ok for trunk?

In the end I like the scheduling code less and less - I believe it will become
a major compile-time hog.  Still,

+      gimple temp = gsi_stmt (gsi);
+      uid = gimple_uid (temp);

no need for 'temp', just write gimple_uid (gsi_stmt (gsi))

+    }
+  if (iters >= MAX_UNASSIGNED_UIDS || uid == 0)
+    renumber_gimple_stmt_uids_in_blocks (&bb, 1);

so this will renumber uids whenever we have trailing zero-UID stmts at the
end of a basic-block?

+  else
+    {
+      for (gsi_prev (&gsi); !gsi_end_p (gsi); gsi_prev (&gsi))
+        {
+          stmt = gsi_stmt (gsi);
+          if (gimple_uid (stmt) > 0)
+            break;
+          gimple_set_uid (stmt, uid);
+        }

while this fills zero-UIDs from the found UID.  A comment on the
re-fill strathegy
would be nice here.

@@ -2901,6 +2930,9 @@ find_insert_point (gimple stmt, gimple dep_stmt)
   gimple insert_stmt = stmt;
   if (dep_stmt == NULL)
     return stmt;
+  ensure_valid_uid (stmt);
+  ensure_valid_uid (dep_stmt);
+
   if (gimple_uid (insert_stmt) == gimple_uid (dep_stmt)

vertical space oddity - move the blank line before the first
ensure_valid_uid () call.

+                        {
+                          tree rhs1 = gimple_assign_rhs1 (stmt);
+                          swap_ops_for_binary_stmt (ops, len - 3, stmt);
+                          regenerate_debug_stmts (SSA_NAME_DEF_STMT
(rhs1), len - 3);
+                        }

I'm not sure this is the right place for this.  Also note my previous
comment that it
is wrong to re-use SSA names for different values because of associated info
that can now be wrong.  Yes, it's a pre-existing bug but you end up exposing it.

I believe the fix is in the places that adjust a stmts rhs1/rhs2.  Yes, reassoc
is a complete mess with regard to this as it modifies the IL for its analysis
purposes.

We'll have interesting times dealing with all this now that we preserve
value-range information for SSA names (which definitely is garbled by
reassoc now).

We absolutely have to do something about this for 4.9 and I think I
shouldn't have
approved the original scheduling work :/  Which complicates matters more,
similar to the || and && reassoc work, and makes the needed rewrite a much
more complex task.

Bah.

Richard.

>> Thanks,
>> Easwaran
>>
>> 2013-09-16  Easwaran Raman  <eraman@google.com>
>>
>>         PR middle-end/57393
>>         * tree-ssa-reassoc.c (get_stmt_uid_with_default): Remove.
>>         (build_and_add_sum): Do not set UID of newly created statements.
>>         (ensure_valid_uid): New function,
>>         (find_insert_point): called here before UIDs are compared.
>>         (insert_stmt_after): Do not reset debug statements.
>>         (regenerate_debug_stmts): New function,
>>         (reassociate_bb): use here.
>>
>> testsuite/ChangeLog:
>> 2013-09-16  Easwaran Raman  <eraman@google.com>
>>
>>         PR middle-end/57393
>>         * gcc.dg/tree-ssa/reassoc-32.c: New testcase.
>>         * gcc.dg/tree-ssa/reassoc-33.c: New testcase.
>>         * gcc.dg/tree-ssa/reassoc-34.c: New testcase.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]