On Mon, Sep 23, 2013 at 5:56 PM, Easwaran Raman <era...@google.com> wrote:
> Ping.
>
>
> On Mon, Sep 16, 2013 at 4:42 PM, Easwaran Raman <era...@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 <era...@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 <era...@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.
