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] |
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] |