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.
