| Message ID | 0DA23CC379F5F945ACB41CF394B9827720F341BF@LEMAIL01.le.imgtec.org |
|---|---|
| State | New |
| Headers | show |
> 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
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
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 R10000 errata. */ mips_branch_likely = TARGET_FIX_R10000; + 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 R10000 + errata. */ + mips_branch_likely = TARGET_FIX_R10000; mips_process_sync_loop (insn, operands); return mips_multi_num_insns; }
> Yes, removing the second NOP is worth the additional effort. The updated patch is below. Ok to commit? Regards, Andrew