This is the mail archive of the
binutils@sources.redhat.com
mailing list for the binutils project.
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