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 Wed, Oct 15, 2003 at 12:13:09AM -0700, Jim Wilson wrote:
> 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.

It is trickier than I thought. Here is a patch. I had to add

 // unconditional compares generate a mutex
-(p3)   cmp.eq.unc p1, p2 = r1, r2
+(p3)   cmp.eq.unc p1, p2 = r1, r2;;

Without the stop bit, the new assembler will complain.


H.J.

Attachment: gas-ia64-dv-pred-7.patch
Description: Text document


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