On Tue, Oct 22, 2013 at 12:47:41PM -0600, Jeff Law wrote:
> On 10/22/13 07:09, Jakub Jelinek wrote:
> >I've spent over two days looking at reassoc, fixing spots where
> >we invalidly reused SSA_NAMEs (this results in wrong-debug, as the added
> >guality testcases show, even some ICEs (pr58791-3.c) and wrong range info
> >for SSA_NAMEs)
> This is something we all need to remember, directly reusing an
> existing SSA_NAME for a new value and the like this is bad. It's
> far better to release the name back to the manager and grab a new
> one.
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.
> >-/* 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.
If bb2 is NULL and bb1 is not, then s2 (assumed to be at the beginning of
function) certainly doesn't dominate s1, if bb2 is not NULL and bb1 is, then
s1 (assumed to be at the beginning of function) dominates s2, if both bb1
and bb2 are NULL, then both are assumed to be at the same spot, so like the
s1 == s2 case.
Note, the if (!bb1 || s1 == s2) return true; comes from the
tree-ssa-loop-niter.c origin, if (!bb2) return false; does not.
> >+ if (bb1 == bb2)
> >+ {
> >+ if (gimple_code (s2) == GIMPLE_PHI)
> >+ return false;
> >+
> >+ if (gimple_code (s1) == GIMPLE_PHI)
> >+ return true;
> Deserves a comment. I know what's going on here, but it's easier to
> read it in a comment rather than recalling the all-phis-in-parallel
> rule and verifying this handles that correctly.
Ok.
> >+ 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).
> >+ 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.
>
>
> >+
> >+/* Insert STMT after INSERT_POINT. */
> >+
> >+static void
> >+insert_stmt_after (gimple stmt, gimple insert_point)
> > {
> >- return stmt ? gimple_uid (stmt) : 1;
> >+ gimple_stmt_iterator gsi;
> >+ basic_block bb;
> >+
> >+ if (gimple_code (insert_point) == GIMPLE_PHI)
> >+ bb = gimple_bb (insert_point);
> >+ else if (!stmt_ends_bb_p (insert_point))
> >+ {
> >+ gsi = gsi_for_stmt (insert_point);
> >+ gimple_set_uid (stmt, gimple_uid (insert_point));
> >+ gsi_insert_after (&gsi, stmt, GSI_NEW_STMT);
> >+ return;
> >+ }
> >+ else
> >+ bb = find_fallthru_edge (gimple_bb (insert_point)->succs)->dest;
> >+ gsi = gsi_after_labels (bb);
> >+ if (gsi_end_p (gsi))
> >+ {
> >+ gimple_stmt_iterator gsi2 = gsi_last_bb (bb);
> >+ gimple_set_uid (stmt,
> >+ gsi_end_p (gsi2) ? 1 : gimple_uid (gsi_stmt (gsi2)));
> >+ }
> >+ else
> >+ gimple_set_uid (stmt, gimple_uid (gsi_stmt (gsi)));
> >+ gsi_insert_before (&gsi, stmt, GSI_SAME_STMT);
> So from reading the name of the function, it's not clear that this
> inserts on the fallthru edge in the case where INSERT_POINT is the
> end of a block. And does that make sense? ISTM you'd want it on
> all the outgoing edges. And you have to worry about things like
> abnormals, critical edges, etc.
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.
Anyway, from my understanding, insert_point is always a SSA_NAME setter (the
only reason to insert something after it is because the something uses the
SSA_NAME as one of it's operands), so it can't be say
GIMPLE_COND/GIMPLE_SWITCH with multiple edges, it can be IMHO only a call
with LHS that could throw, or some assignment with LHS that could throw.
But, given that if a statement throws, the return value/result of operation
isn't really assigned, I believe it is safe to insert only on the fallthru
edge, one can't insert on abnormal edges anyway and the EH/abnormal edge
destination shouldn't use the SSA_NAME anyway, because it will not have any
meaningful value.
Jakub
