On 10/23/13 04:35, Jakub Jelinek wrote:
For debug info quality it actually isn't just about using different
SSA_NAME, but also not reusing the defining stmt; only then the code
will magically try to create debug temporaries and expressions from the old
dead defining stmt.
Joys. Something else to remember.
-/* Returns the UID of STMT if it is non-NULL. Otherwise return 1. */
+/* Returns true if statement S1 dominates statement S2. Like
+ stmt_dominates_stmt_p, but uses stmt UIDs to optimize. */
-static inline unsigned
-get_stmt_uid_with_default (gimple stmt)
+static bool
+reassoc_stmt_dominates_stmt_p (gimple s1, gimple s2)
+{
+ basic_block bb1 = gimple_bb (s1), bb2 = gimple_bb (s2);
+
+ if (!bb1 || s1 == s2)
+ return true;
+
+ if (!bb2)
+ return false;
Maybe this was carried over from somewhere else, but that looks
awful strange. When !bb1 you return true, but if !bb2 you return
false. At the least it deserves a comment.
bb{1,2} == NULL means a default definition of the corresponding lhs.
Ahh, it makes much more sense now.
ISTM that should be documented in the function header and in
stmt_dominates_stmt_p in. Yes, I know you're not responsible for this
mess, but better to get it cleaned up now since you're in there.
Obviously no need for a re-bootstrap to add that comment fix.
+ if (gimple_uid (s1) < gimple_uid (s2))
+ return true;
+
+ if (gimple_uid (s1) > gimple_uid (s2))
+ return false;
So if one (but not both) of the UIDs isn't set yet, one of these two
statements will return, which seems wrong since we don't know where
the statement without a UID is relative to the statement with a UID.
Am I missing something?
Right now we shouldn't see an unset uid here at all, unless it is a PHI,
which is handled earlier. break_up_subtract_bb initializes them for
all preexisting stmts, and the various places which add new stmts are
responsible for setting uid to either the uid of the immediately preceeding
or following stmt that has uid set (or 1 if the bb was previously empty).
OK. Might be worth a comment as well -- something as simple as only
PHIs have unset UIDs here. Once that precondition is known the code is
pretty easy to understand.
+ gimple_stmt_iterator gsi = gsi_for_stmt (s1);
+ unsigned int uid = gimple_uid (s1);
+ for (gsi_next (&gsi); !gsi_end_p (gsi); gsi_next (&gsi))
+ {
+ gimple s = gsi_stmt (gsi);
+ if (gimple_uid (s) != uid)
+ break;
+ if (s == s2)
+ return true;
+ }
Don't we only get here if both UIDs are zero (or the same by other
means)? If so does this code even make sense?
Both UIDs zero should not happen, both UIDs the same can happen, we don't
renumber the whole bb upon inserting something into the bb.
Ok.
Note the above isn't in any way a new code, just a cleanup of the
preexisting, just earlier the thing was done in more than one place
and could mishandle the uid setting.
I understand that. But I can easily see that the function is either
mis-named, poorly documented, or utter crap. The question is which is
it :-)
Anyway, from my understanding, insert_point is always a SSA_NAME setter
So let's get that documented. Again, once the precondition is
understood, the code makes more sense.
At this point my only concerns are getting those functions documented a
bit better -- which you can do as a pre-approved followup.
Being away from GCC for a long time has actually been good in some ways
in that I don't have experience with much of the new bits. Thus I rely
a lot more on comments, function/variable names, etc to provide some of
the context.
Jeff
