Ping: GAS/ia64: spurious dv conflict
H. J. Lu
hjl@lucon.org
Thu Oct 16 17:31:00 GMT 2003
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.
-------------- next part --------------
gas/
2003-10-14 Andreas Schwab <schwab@suse.de>
H.J. Lu <hongjiu.lu@intel.com>
Jim Wilson <wilson@specifixinc.com>
* config/tc-ia64.c (update_qp_mutex): New.
(note_register_values): Properly handle one of PRs in compare
is PR0. Don't add a mutex relation for .and.orcm/.or.andcm.
Clear mutex relation for .none/.unc. Don't clear mutex relation
on predicated compare.
testsuite/
2003-10-14 Andreas Schwab <schwab@suse.de>
H.J. Lu <hongjiu.lu@intel.com>
* gas/ia64/dv-mutex-err.s: Add more tests for compare.
* gas/ia64/dv-mutex.s: Likewise.
* gas/ia64/dv-mutex-err.l: Updated.
* gas/ia64/dv-mutex.d: Likewise.
--- gas/config/tc-ia64.c.pred 2003-06-30 08:05:22.000000000 -0700
+++ gas/config/tc-ia64.c 2003-10-16 09:48:31.000000000 -0700
@@ -8726,6 +8726,76 @@ clear_qp_branch_flag (mask)
}
}
+/* MASK contains 2 and only 2 PRs which are mutually exclusive. Remove
+ any mutexes which contain one of the PRs and create new ones when
+ needed. */
+
+static int
+update_qp_mutex (valueT mask)
+{
+ int i, j;
+ int add = 0;
+ valueT m;
+
+ i = 0;
+ while (i < qp_mutexeslen)
+ {
+ if ((qp_mutexes[i].prmask & mask) != 0)
+ {
+ /* If it destroys and creates the same mutex, do nothing. */
+ if (qp_mutexes[i].prmask == mask
+ && qp_mutexes[i].path == md.path)
+ {
+ i++;
+ add = -1;
+ }
+ else
+ {
+ if (md.debug_dv)
+ {
+ fprintf (stderr, " Clearing mutex relation");
+ print_prmask (qp_mutexes[i].prmask);
+ fprintf (stderr, "\n");
+ }
+
+ if (qp_mutexes[i].path == md.path)
+ {
+ /* If it is a proper subset of the mutex, create a new
+ mutex. */
+ if ((qp_mutexes[i].prmask & mask) == mask)
+ add = 1;
+
+ m = qp_mutexes[i].prmask & ~mask;
+ for (j = 1; !(m & 1); j++)
+ m >>= 1LL;
+
+ m >>= 1LL;
+ if (m)
+ {
+ /* Modify the mutex if there are more than one PR
+ left. */
+ qp_mutexes[i].prmask &= ~mask;
+ i++;
+ }
+ else
+ /* Remove the mutex. */
+ qp_mutexes[i] = qp_mutexes[--qp_mutexeslen];
+ }
+ else
+ /* Remove the mutex. */
+ qp_mutexes[i] = qp_mutexes[--qp_mutexeslen];
+ }
+ }
+ else
+ ++i;
+ }
+
+ if (add == 1)
+ add_qp_mutex (mask);
+
+ return add;
+}
+
/* Remove any mutexes which contain any of the PRs indicated in the mask.
Any changes to a PR clears the mutex relations which include that PR. */
@@ -8990,11 +9060,11 @@ note_register_values (idesc)
{
int p1 = CURR_SLOT.opnd[0].X_add_number - REG_P;
int p2 = CURR_SLOT.opnd[1].X_add_number - REG_P;
- valueT p1mask = (valueT) 1 << p1;
- valueT p2mask = (valueT) 1 << p2;
+ valueT p1mask = (p1 != 0) ? (valueT) 1 << p1 : 0;
+ valueT p2mask = (p2 != 0) ? (valueT) 1 << p2 : 0;
- /* If one of the PRs is PR0, we can't really do anything. */
- if (p1 == 0 || p2 == 0)
+ /* If both PRs are PR0, we can't really do anything. */
+ if (p1 == 0 && p2 == 0)
{
if (md.debug_dv)
fprintf (stderr, " Ignoring PRs due to inclusion of p0\n");
@@ -9004,7 +9074,6 @@ note_register_values (idesc)
else if (has_suffix_p (idesc->name, ".or.andcm")
|| has_suffix_p (idesc->name, ".and.orcm"))
{
- add_qp_mutex (p1mask | p2mask);
clear_qp_implies (p2mask, p1mask);
}
else if (has_suffix_p (idesc->name, ".andcm")
@@ -9020,26 +9089,29 @@ note_register_values (idesc)
}
else
{
+ int added = 0;
+
clear_qp_implies (p1mask | p2mask, p1mask | p2mask);
- if (has_suffix_p (idesc->name, ".unc"))
+
+ /* If one of the PRs is PR0, we call clear_qp_mutex. */
+ if (p1 == 0 || p2 == 0)
+ clear_qp_mutex (p1mask | p2mask);
+ else
+ added = update_qp_mutex (p1mask | p2mask);
+
+ if (CURR_SLOT.qp_regno == 0
+ || has_suffix_p (idesc->name, ".unc"))
{
- add_qp_mutex (p1mask | p2mask);
+ if (added == 0 && p1 && p2)
+ add_qp_mutex (p1mask | p2mask);
if (CURR_SLOT.qp_regno != 0)
{
- add_qp_imply (CURR_SLOT.opnd[0].X_add_number - REG_P,
- CURR_SLOT.qp_regno);
- add_qp_imply (CURR_SLOT.opnd[1].X_add_number - REG_P,
- CURR_SLOT.qp_regno);
+ if (p1)
+ add_qp_imply (p1, CURR_SLOT.qp_regno);
+ if (p2)
+ add_qp_imply (p2, CURR_SLOT.qp_regno);
}
}
- else if (CURR_SLOT.qp_regno == 0)
- {
- add_qp_mutex (p1mask | p2mask);
- }
- else
- {
- clear_qp_mutex (p1mask | p2mask);
- }
}
}
/* Look for mov imm insns into GRs. */
--- gas/testsuite/gas/ia64/dv-mutex-err.l.pred 2000-04-23 09:41:25.000000000 -0700
+++ gas/testsuite/gas/ia64/dv-mutex-err.l 2003-10-16 10:02:55.000000000 -0700
@@ -2,3 +2,12 @@
.*:9: Warning: Use of 'ld8' .* RAW dependency 'GR%, % in 1 - 127' \(impliedf\), specific resource number is 26
.*:9: Warning: Only the first path encountering the conflict is reported
.*:8: Warning: This is the location of the conflicting usage
+.*:14: Warning: Use of 'mov' may violate WAW dependency 'GR%, % in 1 - 127' \(impliedf\), specific resource number is 4
+.*:14: Warning: Only the first path encountering the conflict is reported
+.*:13: Warning: This is the location of the conflicting usage
+.*:20: Warning: Use of 'mov' may violate WAW dependency 'GR%, % in 1 - 127' \(impliedf\), specific resource number is 4
+.*:19: Warning: This is the location of the conflicting usage
+.*:26: Warning: Use of 'mov' may violate WAW dependency 'GR%, % in 1 - 127' \(impliedf\), specific resource number is 4
+.*:25: Warning: This is the location of the conflicting usage
+.*:32: Warning: Use of 'mov' may violate WAW dependency 'GR%, % in 1 - 127' \(impliedf\), specific resource number is 4
+.*:31: Warning: This is the location of the conflicting usage
--- gas/testsuite/gas/ia64/dv-mutex-err.s.pred 2000-04-23 09:41:25.000000000 -0700
+++ gas/testsuite/gas/ia64/dv-mutex-err.s 2003-10-16 10:02:49.000000000 -0700
@@ -7,3 +7,27 @@ start:
cmp.eq p6, p0 = r29, r0
add r26 = r26, r29
ld8 r29 = [r26]
+
+ .pred.rel.mutex p1, p2
+ cmp.eq p0, p1 = r1, r2;;
+(p1) mov r4 = 2
+(p2) mov r4 = 4
+ rfi
+
+ .pred.rel.mutex p1, p2
+(p3) cmp.eq p0, p1 = r1, r2;;
+(p1) mov r4 = 2
+(p2) mov r4 = 4
+ rfi
+
+ .pred.rel.mutex p1, p2
+ cmp.eq p2, p3 = r1, r2;;
+(p1) mov r4 = 2
+(p2) mov r4 = 4
+ rfi
+
+ .pred.rel.mutex p1, p2
+(p3) cmp.eq p2, p3 = r1, r2;;
+(p1) mov r4 = 2
+(p2) mov r4 = 4
+ rfi
--- gas/testsuite/gas/ia64/dv-mutex.d.pred 2003-09-25 21:07:48.000000000 -0700
+++ gas/testsuite/gas/ia64/dv-mutex.d 2003-10-14 22:03:47.000000000 -0700
@@ -19,9 +19,21 @@ Disassembly of section \.text:
30: 1d 00 00 00 01 00 \[MFB\] nop\.m 0x0
36: 00 00 00 02 00 00 nop\.f 0x0
3c: 00 00 20 00 rfi;;
- 40: 60 08 06 04 02 78 \[MII\] \(p03\) cmp\.eq\.unc p1,p2=r1,r2
+ 40: 6a 08 06 04 02 78 \[MMI\] \(p03\) cmp\.eq\.unc p1,p2=r1,r2;;
46: 40 10 00 00 42 81 \(p01\) mov r4=2
4c: 40 00 00 84 \(p02\) mov r4=4
50: 1d 00 00 00 01 00 \[MFB\] nop\.m 0x0
56: 00 00 00 02 00 00 nop\.f 0x0
5c: 00 00 20 00 rfi;;
+ 60: 0a 08 04 04 02 78 \[MMI\] cmp\.eq p1,p2=r1,r2;;
+ 66: 40 10 00 00 42 81 \(p01\) mov r4=2
+ 6c: 40 00 00 84 \(p02\) mov r4=4
+ 70: 1d 00 00 00 01 00 \[MFB\] nop\.m 0x0
+ 76: 00 00 00 02 00 00 nop\.f 0x0
+ 7c: 00 00 20 00 rfi;;
+ 80: 6a 08 04 04 02 78 \[MMI\] \(p03\) cmp\.eq p1,p2=r1,r2;;
+ 86: 40 10 00 00 42 81 \(p01\) mov r4=2
+ 8c: 40 00 00 84 \(p02\) mov r4=4
+ 90: 1d 00 00 00 01 00 \[MFB\] nop\.m 0x0
+ 96: 00 00 00 02 00 00 nop\.f 0x0
+ 9c: 00 00 20 00 rfi;;
--- gas/testsuite/gas/ia64/dv-mutex.s.pred 2000-09-02 09:58:33.000000000 -0700
+++ gas/testsuite/gas/ia64/dv-mutex.s 2003-10-16 10:11:16.000000000 -0700
@@ -17,7 +17,19 @@ start:
rfi
// unconditional compares generate a mutex
-(p3) cmp.eq.unc p1, p2 = r1, r2
+(p3) cmp.eq.unc p1, p2 = r1, r2;;
+(p1) mov r4 = 2
+(p2) mov r4 = 4
+ rfi
+
+// non-predicated compares don't remove mutex
+ cmp.eq p1, p2 = r1, r2;;
+(p1) mov r4 = 2
+(p2) mov r4 = 4
+ rfi
+
+// predicated compares don't remove mutex
+(p3) cmp.eq p1, p2 = r1, r2;;
(p1) mov r4 = 2
(p2) mov r4 = 4
rfi
More information about the Binutils
mailing list