This is the mail archive of the binutils@sources.redhat.com mailing list for the binutils 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: Ping: GAS/ia64: spurious dv conflict


On Mon, 2003-10-20 at 09:12, H. J. Lu wrote:
> Here is the updated patch.

I don't like the way that you keep trying to fix more and more bugs with
a single patch.  This would be much easier to review if you had separate
patches for separate bugs.  Your gas-ia64-dv-pred-6.patch looked mostly
OK to me except that I disagreed with some of your testsuite changes . 
But now I have a new patch to review rather than a fixed one, and by
introducing new issues, you have added new complications.

I didn't go into details about the issue of mutexes of 3 or more
predicate registers because I was expecting you to fix that with a
separate patch.  I also hadn't thought about it much at the time.

I said last week that we didn't properly handle mutexes that involved
more than two predicate registers, and that I thought we needed special
code to check for and handle this in update_qp_mutex and other
routines.  However, over the weekend, it occurred to me that there is a
better way to fix this.  ".pred.rel.mutex  p1, p2, p3" is equivalent to
".pred.rel.mutex p1,p2; .pred.rel.mutex p2,p3; .pred.rel.mutex p1,p3". 
We represent the former with a single qp_mutexes entry with 3 bits set,
and we represent the latter with 3 qp_mutexes entries with 2 bits set
each.  This requires special checks for for qp_mutexes entries with 3 or
more bits set which we currently don't have.  I think a better solution
would be to change the representation, to always have mutexes in a
canonical form.  This means that if we are given .pred.rel.mutex with
more than 2 PRs, then we create multiple qp_mutexes entries instead of a
single one with >2 bits set.  This avoids the need for special code in
update_qp_mutex, and also solves some other problems.  I believe that we
will eventually have to change the underlying representation for mutexes
in order to fix some of the known DV bugs.  We will have to change
qp_mutex from a list of mutexes to a 2 dimensional array (indexed by prX
vs prY).  When we do this, we will no longer be able to represent
mutexes of more than 2 PRs with a single entry, so we might as well get
rid of that abstraction now.  This also has the advantage that we only
need to fix one place: dot_pred_rel.  If we don't change the
representation, then we need to fix every place that accesses the
qp_mutex info, which means update_qp_mutex, but also clear_qp_mutex, and
probably some other routines.

As for the patch:

The code for checking for a prmask with more than one bit set is
unnecessarily complex.  There is no need for a loop here.  All you need
is
	qp_mutexes[i].prmask &= ~mask;
        if (qp_mutexes[i].prmask & (qp_mutexes[i].prmask - 1) == 0)
	  ...delete it...
	else
	  ...keep it...
You can get rid of about 25 lines of code and some local variables if
you use this trick.

You have fixed update_qp_mutex to handle a prmask with more than 2 bits
set.  But you haven't fixed clear_qp_mutex, or any other affected
routines.  So this is an incomplete fix for the problem.

Also, as mentioned above, I now think fixing dot_pred_rel() is a better
solution for this problem, which makes these latest update_qp_mutex
changes unnecessary.

Why are you checking qp_mutexes[i].path in the new patch?  You didn't
add any comments in the patch or explanation in the email message.  This
does look right to me, but it is non-obvious, since both of us missed
this in earlier patches.

The only other substantial change if that update_qp_mutex returns
non-zero if a mutex was kept or added, and then you avoid calling
add_qp_mutex in that case, which prevents us from adding duplicate
mutexes.  So this looks OK.  However, this is unnecessary if we fix
dot_pred_rel instead, and I think that is a better solution.

The testsuite changes all look OK to me.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com


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