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

[Jeff Law]

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