On Wed, 23 Oct 2013, Jakub Jelinek wrote:
> 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.
I'm ok with the patch. Not sure if we want to backport it at all - we
will only have wrong-debug issues (well, "only").
Thanks,
Richard.
