This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix various reassoc issues (PR tree-optimization/58791, tree-optimization/58775)


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]