Re: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
FWIW, I agree this is the right fix, but: Andrew Bennett andrew.benn...@imgtec.com writes: + /* When using branch likely (-mfix-r1), the delay slot instruction + will be annulled on false. The normal delay slot instructions + calculate the overall result of the atomic operation and must not + be annulled. To ensure this behaviour unconditionally use a NOP + in the delay slot for the branch likely case. */ remember to use US spelling: behavior rather than behaviour. Thanks, Richard
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
Ok to commit? gcc/ * config/mips/mips.c (mips_process_sync_loop): Place a nop in the delay slot of the branch likely instruction. With an updated ChangeLog to account for the changes in the callers, OK. Matthew
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
-Original Message- From: Matthew Fortune [mailto:matthew.fort...@imgtec.com] Sent: Tuesday, November 18, 2014 9:42 AM To: Andrew Bennett; gcc-patches@gcc.gnu.org Cc: Moore, Catherine; Rozycki, Maciej Subject: RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop The atomic-compare-exchange-3.c and atomic-op-3.c tests are failing when using the -mfix-r1 option. This is due to the fact that the delay slot of the branch instruction that checks if the atomic operation was not successful can be filled with an operation that returns the output result. For the branch likely case this operation can not go in the delay slot because it wont execute when the atomic operation was successful and therefore the wrong result will be returned. This patch forces a nop to be placed in the delay slot if a branch likely is used. The fix is as simple as possible; it does cause a second nop to be introduced after the branch instruction in the branch likely case. As this option is rarely used, it makes more sense to keep the code readable than trying to optimise it. The atomic tests now pass successfully. The ChangeLog and patch are below. Ok to commit? I'm OK with just making the fix-r1 case safe rather than also complicating the normal code path to remove the normal delay slot NOP in this special case. I'd like to see what Catherine thinks though. To remove the second NOP I believe we would have to add a !TARGET_FIX_R1 to the condition of the normal delay slot NOP. This seems a bit convoluted when the real reason is because branch likely is in use. To make use of the mips_branch_likely flag it would have to be set earlier in: mips_output_sync_loop and also get set in mips_sync_loop_insns. If Catherine thinks getting rid of the second NOP is important enough then please base it on mips_branch_likely and fix the callers. Yes, removing the second NOP is worth the additional effort.
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
Yes, removing the second NOP is worth the additional effort.
The updated patch is below.
Ok to commit?
Regards,
Andrew
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 02268f3..368c6f0 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -12997,7 +12997,12 @@ mips_process_sync_loop (rtx_insn *insn, rtx *operands)
This will sometimes be a delayed branch; see the write code below
for details. */
mips_multi_add_insn (is_64bit_p ? scd\t%0,%1 : sc\t%0,%1, at, mem, NULL);
- mips_multi_add_insn (beq%?\t%0,%.,1b, at, NULL);
+
+ /* We can not put the NEWVAL = $TMP3 or CMP = 1 operations in the delay slot
+ of the branch if it is a branch likely because they will not execute when
+ the atomic operation is successful, so place a NOP in there instead. */
+
+ mips_multi_add_insn (beq%?\t%0,%.,1b%~, at, NULL);
/* if (INSN1 != MOVE INSN1 != LI) NEWVAL = $TMP3 [delay slot]. */
if (insn1 != SYNC_INSN1_MOVE insn1 != SYNC_INSN1_LI tmp3 != newval)
@@ -13005,7 +13010,7 @@ mips_process_sync_loop (rtx_insn *insn, rtx *operands)
mips_multi_copy_insn (tmp3_insn);
mips_multi_set_operand (mips_multi_last_index (), 0, newval);
}
- else if (!(required_oldval cmp))
+ else if (!(required_oldval cmp) !mips_branch_likely)
mips_multi_add_insn (nop, NULL);
/* CMP = 1 -- either standalone or in a delay slot. */
@@ -13029,12 +13034,12 @@ mips_process_sync_loop (rtx_insn *insn, rtx *operands)
const char *
mips_output_sync_loop (rtx_insn *insn, rtx *operands)
{
- mips_process_sync_loop (insn, operands);
-
/* Use branch-likely instructions to work around the LL/SC R1
errata. */
mips_branch_likely = TARGET_FIX_R1;
+ mips_process_sync_loop (insn, operands);
+
mips_push_asm_switch (mips_noreorder);
mips_push_asm_switch (mips_nomacro);
mips_push_asm_switch (mips_noat);
@@ -13056,6 +13061,9 @@ mips_output_sync_loop (rtx_insn *insn, rtx *operands)
unsigned int
mips_sync_loop_insns (rtx_insn *insn, rtx *operands)
{
+ /* Use branch-likely instructions to work around the LL/SC R1
+ errata. */
+ mips_branch_likely = TARGET_FIX_R1;
mips_process_sync_loop (insn, operands);
return mips_multi_num_insns;
}
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index 02268f3..368c6f0 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -12997,7 +12997,12 @@ mips_process_sync_loop (rtx_insn *insn, rtx *operands) This will sometimes be a delayed branch; see the write code below for details. */ mips_multi_add_insn (is_64bit_p ? scd\t%0,%1 : sc\t%0,%1, at, mem, NULL); - mips_multi_add_insn (beq%?\t%0,%.,1b, at, NULL); + + /* We can not put the NEWVAL = $TMP3 or CMP = 1 operations in the delay slot + of the branch if it is a branch likely because they will not execute when + the atomic operation is successful, so place a NOP in there + instead. */ Please rephrase the comment along the lines of my previous suggestion. This wording is too complex IMO. Matthew
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
On Wed, 19 Nov 2014, Matthew Fortune wrote: @@ -12997,7 +12997,12 @@ mips_process_sync_loop (rtx_insn *insn, rtx *operands) This will sometimes be a delayed branch; see the write code below for details. */ mips_multi_add_insn (is_64bit_p ? scd\t%0,%1 : sc\t%0,%1, at, mem, NULL); - mips_multi_add_insn (beq%?\t%0,%.,1b, at, NULL); + + /* We can not put the NEWVAL = $TMP3 or CMP = 1 operations in the delay slot + of the branch if it is a branch likely because they will not execute when + the atomic operation is successful, so place a NOP in there + instead. */ Please rephrase the comment along the lines of my previous suggestion. This wording is too complex IMO. Also I'm not sure if it's an enforced rule for GCC sources (it is for GDB for example; someone please chime in if I'm missing something here), but can you take the opportunity and limit the span of these comment lines a bit, like to 74 or maybe even 72 columns, similarly to the rule for ChangeLog entries, to make them more readable. Thanks. Maciej
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
Please rephrase the comment along the lines of my previous suggestion.
This wording is too complex IMO.
The patch containing the updated comment (which also keeps within 72 columns)
is below.
Ok to commit?
Regards,
Andrew
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 02268f3..bf5682c 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -12997,7 +12997,14 @@ mips_process_sync_loop (rtx_insn *insn, rtx *operands)
This will sometimes be a delayed branch; see the write code below
for details. */
mips_multi_add_insn (is_64bit_p ? scd\t%0,%1 : sc\t%0,%1, at, mem, NULL);
- mips_multi_add_insn (beq%?\t%0,%.,1b, at, NULL);
+
+ /* When using branch likely (-mfix-r1), the delay slot instruction
+ will be annulled on false. The normal delay slot instructions
+ calculate the overall result of the atomic operation and must not
+ be annulled. To ensure this behaviour unconditionally use a NOP
+ in the delay slot for the branch likely case. */
+
+ mips_multi_add_insn (beq%?\t%0,%.,1b%~, at, NULL);
/* if (INSN1 != MOVE INSN1 != LI) NEWVAL = $TMP3 [delay slot]. */
if (insn1 != SYNC_INSN1_MOVE insn1 != SYNC_INSN1_LI tmp3 != newval)
@@ -13005,7 +13012,7 @@ mips_process_sync_loop (rtx_insn *insn, rtx *operands)
mips_multi_copy_insn (tmp3_insn);
mips_multi_set_operand (mips_multi_last_index (), 0, newval);
}
- else if (!(required_oldval cmp))
+ else if (!(required_oldval cmp) !mips_branch_likely)
mips_multi_add_insn (nop, NULL);
/* CMP = 1 -- either standalone or in a delay slot. */
@@ -13029,12 +13036,12 @@ mips_process_sync_loop (rtx_insn *insn, rtx *operands)
const char *
mips_output_sync_loop (rtx_insn *insn, rtx *operands)
{
- mips_process_sync_loop (insn, operands);
-
/* Use branch-likely instructions to work around the LL/SC R1
errata. */
mips_branch_likely = TARGET_FIX_R1;
+ mips_process_sync_loop (insn, operands);
+
mips_push_asm_switch (mips_noreorder);
mips_push_asm_switch (mips_nomacro);
mips_push_asm_switch (mips_noat);
@@ -13056,6 +13063,9 @@ mips_output_sync_loop (rtx_insn *insn, rtx *operands)
unsigned int
mips_sync_loop_insns (rtx_insn *insn, rtx *operands)
{
+ /* Use branch-likely instructions to work around the LL/SC R1
+ errata. */
+ mips_branch_likely = TARGET_FIX_R1;
mips_process_sync_loop (insn, operands);
return mips_multi_num_insns;
}
Re: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
On Tue, 18 Nov 2014, Andrew Bennett wrote: The atomic-compare-exchange-3.c and atomic-op-3.c tests are failing when using the -mfix-r1 option. This is due to the fact that the delay slot of the branch instruction that checks if the atomic operation was not successful can be filled with an operation that returns the output result. For the branch likely case this operation can not go in the delay slot because it wont execute when the atomic operation was successful and therefore the wrong result will be returned. This patch forces a nop to be placed in the delay slot if a branch likely is used. The fix is as simple as possible; it does cause a second nop to be introduced after the branch instruction in the branch likely case. As this option is rarely used, it makes more sense to keep the code readable than trying to optimise it. Can you please be a bit more elaborate on how the wrong code sequence looks like, why it is produced like that and how your fix changes it? Without seeing examples of code generated it is very hard to imagine what is really going on here. Maciej
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
-Original Message-
From: Maciej W. Rozycki [mailto:ma...@codesourcery.com]
Sent: 18 November 2014 13:48
To: Andrew Bennett
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] If using branch likelies in MIPS sync code fill the delay
slot with a nop
On Tue, 18 Nov 2014, Andrew Bennett wrote:
The atomic-compare-exchange-3.c and atomic-op-3.c tests are failing when
using the -mfix-r1 option. This is due to the fact that the delay
slot of the branch instruction that checks if the atomic operation was
not successful can be filled with an operation that returns the output
result. For the branch likely case this operation can not go in the
delay slot because it wont execute when the atomic operation was
successful and therefore the wrong result will be returned. This patch
forces a nop to be placed in the delay slot if a branch likely is used.
The fix is as simple as possible; it does cause a second nop to be
introduced
after the branch instruction in the branch likely case. As this option is
rarely used, it makes more sense to keep the code readable than trying to
optimise it.
Can you please be a bit more elaborate on how the wrong code sequence
looks like, why it is produced like that and how your fix changes it?
Without seeing examples of code generated it is very hard to imagine
what is really going on here.
Ok if we compile the following cut-down atomic-op-3.c test case with
-mfix-r1.
extern void abort(void);
int v, count, res;
int
main ()
{
v = 0;
count = 1;
if (__atomic_add_fetch (v, count, __ATOMIC_RELAXED) != 1)
abort ();
return 0;
}
The following assembly code is produced for the atomic operation:
.setnoat
sync # 15 sync_new_addsi/2[length = 24]
1:
ll $3,0($4)
addu$1,$3,$2
sc $1,0($4)
beql$1,$0,1b
addu$3,$3,$2
sync
.setat
Notice here that the addu is in the delay slot of the branch likely instruction.
This is computing the value that exists in the memory location after the atomic
operation has completed. Because it is a branch likely instruction
this will only run when the atomic operation fails, and hence when it is
successful it will not return the correct value.
The offending code is in mips_process_sync_loop:
mips_multi_add_insn (beq%?\t%0,%.,1b, at, NULL);
/* if (INSN1 != MOVE INSN1 != LI) NEWVAL = $TMP3 [delay slot]. */
if (insn1 != SYNC_INSN1_MOVE insn1 != SYNC_INSN1_LI tmp3 != newval)
{
mips_multi_copy_insn (tmp3_insn);
mips_multi_set_operand (mips_multi_last_index (), 0, newval);
}
else if (!(required_oldval cmp))
mips_multi_add_insn (nop, NULL);
/* CMP = 1 -- either standalone or in a delay slot. */
if (required_oldval cmp)
mips_multi_add_insn (li\t%0,1, cmp, NULL);
For the branch likely case the delay slot could be filled either with the
operation to compute the value that exists in memory after the atomic operation
has completed; an operation to return if the atomic operation is successful
or not; or a nop. The first two operations should not be in the delay slot
of the branch if it is a branch likely because they will only run if the atomic
operation was unsuccessful.
My fix places a nop in the delay slot of the branch likely instruction
by using the %~ output operation. This then causes the sync code for the
previous example to be correct:
.setnoat
sync # 15 sync_new_addsi/2[length = 24]
1:
ll $3,0($4)
addu$1,$3,$2
sc $1,0($4)
beql$1,$0,1b
nop
addu$3,$3,$2
sync
.setat
Regards,
Andrew
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
The atomic-compare-exchange-3.c and atomic-op-3.c tests are failing when using the -mfix-r1 option. This is due to the fact that the delay slot of the branch instruction that checks if the atomic operation was not successful can be filled with an operation that returns the output result. For the branch likely case this operation can not go in the delay slot because it wont execute when the atomic operation was successful and therefore the wrong result will be returned. This patch forces a nop to be placed in the delay slot if a branch likely is used. The fix is as simple as possible; it does cause a second nop to be introduced after the branch instruction in the branch likely case. As this option is rarely used, it makes more sense to keep the code readable than trying to optimise it. The atomic tests now pass successfully. The ChangeLog and patch are below. Ok to commit? I'm OK with just making the fix-r1 case safe rather than also complicating the normal code path to remove the normal delay slot NOP in this special case. I'd like to see what Catherine thinks though. To remove the second NOP I believe we would have to add a !TARGET_FIX_R1 to the condition of the normal delay slot NOP. This seems a bit convoluted when the real reason is because branch likely is in use. To make use of the mips_branch_likely flag it would have to be set earlier in: mips_output_sync_loop and also get set in mips_sync_loop_insns. If Catherine thinks getting rid of the second NOP is important enough then please base it on mips_branch_likely and fix the callers. gcc/ * config/mips/mips.c (mips_process_sync_loop): Place a nop in the delay slot of the branch likely instruction. diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index 02268f3..6dd3728 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -12997,7 +12997,12 @@ mips_process_sync_loop (rtx_insn *insn, rtx *operands) This will sometimes be a delayed branch; see the write code below for details. */ mips_multi_add_insn (is_64bit_p ? scd\t%0,%1 : sc\t%0,%1, at, mem, NULL); - mips_multi_add_insn (beq%?\t%0,%.,1b, at, NULL); + + /* We can not put the NEWVAL = $TMP3 or CMP = 1 operations in the delay slot + of the branch if it is a branch likely because they will not execute when + the atomic operation is successful, so place a NOP in there instead. */ + I found the comment hard to digest, perhaps: /* When using branch likely (-mfix-r1), the delay slot instruction will be annulled on false. The normal delay slot instructions calculate the overall result of the atomic operation and must not be annulled. Unconditionally use a NOP instead for the branch likely case. */ + mips_multi_add_insn (beq%?\t%0,%.,1b%~, at, NULL); /* if (INSN1 != MOVE INSN1 != LI) NEWVAL = $TMP3 [delay slot]. */ if (insn1 != SYNC_INSN1_MOVE insn1 != SYNC_INSN1_LI tmp3 != newval) Matthew
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
On Tue, 18 Nov 2014, Andrew Bennett wrote: My fix places a nop in the delay slot of the branch likely instruction by using the %~ output operation. This then causes the sync code for the previous example to be correct: .setnoat sync # 15 sync_new_addsi/2[length = 24] 1: ll $3,0($4) addu$1,$3,$2 sc $1,0($4) beql$1,$0,1b nop addu$3,$3,$2 sync .setat OK, this does look to me like the correct way to address the issue, but where is the second NOP that you previously mentioned? I fail to see it here and this code can't be made any better, there isn't anything you could possibly schedule into the delay slot as there is nothing else to do in this loop. Maciej
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
OK, this does look to me like the correct way to address the issue, but
where is the second NOP that you previously mentioned? I fail to see it
here and this code can't be made any better, there isn't anything you
could possibly schedule into the delay slot as there is nothing else to
do in this loop.
The following testcase shows this occurring.
short v, count, ret;
int
main ()
{
v = 0;
count = 0;
__atomic_exchange_n (v, count + 1, __ATOMIC_RELAXED);
return 0;
}
Produces (for the atomic operation):
.setnoat
sync
1:
ll $3,0($5)
and $1,$3,$4
bne $1,$7,2f
and $1,$3,$6
or $1,$1,$8
sc $1,0($5)
beql$1,$0,1b
nop
nop
sync
2:
.setat
Regards,
Andrew
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
On Tue, 18 Nov 2014, Andrew Bennett wrote: Produces (for the atomic operation): .setnoat sync 1: ll $3,0($5) and $1,$3,$4 bne $1,$7,2f and $1,$3,$6 or $1,$1,$8 sc $1,0($5) beql$1,$0,1b nop nop sync 2: .setat With a quick look at `mips_process_sync_loop' it looks to me the other NOP is produced from here: else if (!(required_oldval cmp)) mips_multi_add_insn (nop, NULL); -- correct? If so, then can't you just make it: else if (!(required_oldval cmp) !TARGET_FIX_R1) mips_multi_add_insn (nop, NULL); instead? It appears plain and simple, and if you're concerned about it being unobvious to the casual reader, then add a one-line comment or suchlike. It's not like TARGET_FIX_R1 is going to imply anything else about branches in the future (and r6 code won't run on an R10k anyway). Maciej
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
From: Maciej W. Rozycki [mailto:ma...@codesourcery.com] On Tue, 18 Nov 2014, Andrew Bennett wrote: Produces (for the atomic operation): .setnoat sync 1: ll $3,0($5) and $1,$3,$4 bne $1,$7,2f and $1,$3,$6 or $1,$1,$8 sc $1,0($5) beql$1,$0,1b nop nop sync 2: .setat With a quick look at `mips_process_sync_loop' it looks to me the other NOP is produced from here: else if (!(required_oldval cmp)) mips_multi_add_insn (nop, NULL); -- correct? If so, then can't you just make it: Correct. else if (!(required_oldval cmp) !TARGET_FIX_R1) mips_multi_add_insn (nop, NULL); instead? It appears plain and simple, and if you're concerned about it being unobvious to the casual reader, then add a one-line comment or suchlike. It's not like TARGET_FIX_R1 is going to imply anything else about branches in the future (and r6 code won't run on an R10k anyway). True. Actually this is what my original patch had in it, but Matthew and I decided to leave it out for the initial submission. I am happy to add it back in, and provide a comment to explain what is happening. Catherine are you happy with this? Regards, Andrew
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
From: Maciej W. Rozycki [mailto:ma...@codesourcery.com] On Tue, 18 Nov 2014, Andrew Bennett wrote: Produces (for the atomic operation): .setnoat sync 1: ll $3,0($5) and $1,$3,$4 bne $1,$7,2f and $1,$3,$6 or $1,$1,$8 sc $1,0($5) beql$1,$0,1b nop nop sync 2: .setat With a quick look at `mips_process_sync_loop' it looks to me the other NOP is produced from here: else if (!(required_oldval cmp)) mips_multi_add_insn (nop, NULL); -- correct? If so, then can't you just make it: Correct. else if (!(required_oldval cmp) !TARGET_FIX_R1) mips_multi_add_insn (nop, NULL); instead? It appears plain and simple, and if you're concerned about it being unobvious to the casual reader, then add a one-line comment or suchlike. It's not like TARGET_FIX_R1 is going to imply anything else about branches in the future (and r6 code won't run on an R10k anyway). I'm afraid I disagree about it being plain and simple even with a comment. The amount of code in the backend that makes assumptions based on derived variables is very high and it makes the code hard to read (especially w.r.t. branches). I'm OK with adding the code to avoid the extra NOP but have it based on mips_branch_likely and fix the callers so that it is set appropriately. Thanks, Matthew
RE: [PATCH] If using branch likelies in MIPS sync code fill the delay slot with a nop
On Tue, 18 Nov 2014, Matthew Fortune wrote: With a quick look at `mips_process_sync_loop' it looks to me the other NOP is produced from here: else if (!(required_oldval cmp)) mips_multi_add_insn (nop, NULL); -- correct? If so, then can't you just make it: Correct. else if (!(required_oldval cmp) !TARGET_FIX_R1) mips_multi_add_insn (nop, NULL); instead? It appears plain and simple, and if you're concerned about it being unobvious to the casual reader, then add a one-line comment or suchlike. It's not like TARGET_FIX_R1 is going to imply anything else about branches in the future (and r6 code won't run on an R10k anyway). I'm afraid I disagree about it being plain and simple even with a comment. The amount of code in the backend that makes assumptions based on derived variables is very high and it makes the code hard to read (especially w.r.t. branches). I'm OK with adding the code to avoid the extra NOP but have it based on mips_branch_likely and fix the callers so that it is set appropriately. Sure, fine with me too. It looks to me it'll be more natural even to have `mips_branch_likely' preset on entering `mips_process_sync_loop'. Maciej
