Home Original page

Ping: GAS/ia64: spurious dv conflict

Jim Wilson wilson@specifixinc.com
Wed Oct 15 07:13:00 GMT 2003 
On Tue, 2003-10-14 at 22:18, H. J. Lu wrote:
>         .pred.rel "mutex", p1, p2, p3
> (p3)    cmp.eq p1, p2 = r1, r2;;
> (p1)    mov r4 = 2
> (p2)    mov r4 = 4

The fact that we don't handle mutexes of more than two PRs correctly is
a known problem, and is one of the 20 or so known problems I mentioned
to you a few months ago.  I didn't mention it in this case because I
didn't want this patch to get too complicated.

I disagree with your reasoning here.  In this case, p1/p2 are mutex
before the compare, and are still mutex after the compare, so it is
wrong to give a warning.  In this regard, it should be no different than
the case where the original mutex contained only p1/p2.  You are right
that we need to destroy the p1/p3 and p2/p3 mutexes though.

We can make this issue here a little clearer if we change the example to
be
	.pred.rel "mutex" p1, p2, p3, p4
This defines 6 separate mutex relationships which we represent as one
qp_mutex entry.  After the compare, p3 and p4 are still mutex because
neither was set by the compare.  However, the current code gets this
wrong, and all of your patches get this wrong.  The only way to get this
right is to clear the p1 and p2 bits, then check to see if we still have
two or more bits set, and if not, then we delete the qp_mutex entry. 
Meanwhile, we should preserve the p1/p2 mutex because they are still
guaranteed to be mutex after the compare.  So we should split the single
p1/p2/p3/p4 mutex into two mutexes after the compare, one for p3/p4, and
one for p1/p2.  This makes the code a bit more complicated
unfortunately.

I very strongly recommend against trying to fix this problem in the
current patch.  The current patch is already trying to fix several
different problems in a single patch.  We should not make it any more
complicated.

> Here is another update. We should warn

The update_qp_mutex comment says mask contains mutex PRs, but doesn't
say how many.  I believe this can only work correctly if mask contains
two and only two mutex PRs.  Otherwise, the required calculations get
too complicated.  I mentioned this in my last review.  I think that
should be documented, and optionally even checked for in the code.  The
current code always calls it with two mutex PRs, so this isn't a problem
with the code, just with the documentation.  This is a pretty minor
point though.

You can optionally delete a brace level in the .or.andcm/.and.orcm if
statment.  This is a very minor point.

I disagree with the last testcase you added to dv-mutex-err.s, which has
the .pred.rel.mutex p1,p2,p3 line.  I think this is not a mutex error,
and it is a bad idea to hard wire this result into the testsuite now,
because we would like to get it correct in the future.  I think this
should be left for a future patch.  If you want to add a testcase now, I
think it has to go in a separate file, so we can mark it as an expected
fail.  I don't see the advantages of doing that though.  I think it is
simpler to just leave out that testcase.

I disagree with some of your dv-mutex.s changes also.  For the existing
cmp.eq.unc test, you added a .pred.rel.mutex line.  But this invalidates
what the test is for.  Because a .unc compare always sets the predicate
registers regardless of the qualifying predicate, a conditional .unc
compare does create a mutex.  That is what we are testing here.  Adding
the .pred.rel.mutex line before the cmp.eq.unc changes this from a test
of mutex creation to a test of mutex preservation which was not the
original intent.  If you want a test for mutex preservation, you need to
add a separate test for that.  However, I think such a test is spurious,
because mutex creation trumps mutex preservation, so you can't actually
test for mutex preservation.  It wouldn't hurt to have a test for it
though.

Also, I think it is a bad idea to use .pred.rel.mutex lines that mention
3 predicates here, because that is not what this patch fixes.  That is a
separate problem that is not and should not be addressed by this patch.

The second new testcase in the dv-mutex.s file is ambiguous because of
this problem.  The comment says that non-predicated compares don't
remove mutex, but the pred.rel.mutex line defines 3 separate mutex
relations two of which are removed by the compare.

Otherwise it looks good to me.

At this point, I think I should be responding with modified patches to
make my points clear, but it is after midnight here, so I will do that
tomorrow.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com
More information about the Binutils mailing list