Re: [PATCH] GAS/MIPS: Add `-mfix-r5900' option for the R5900 short loop errata

["Maciej W. Rozycki" <macro at linux-mips dot org>]

Hi Fredrik,

 Thank you for your contribution.  Your proposed change looks mostly good 
to me, but I have one semantic change request, and then the usual bunch of 
minor nits.

> diff --git a/gas/config/tc-mips.c b/gas/config/tc-mips.c
> index 918525b4e9..358c59f837 100644
> --- a/gas/config/tc-mips.c
> +++ b/gas/config/tc-mips.c
> @@ -6997,7 +7004,7 @@ can_swap_branch_p (struct mips_cl_insn *ip, expressionS *address_expr,
>       - a branch delay slot of the loop is not NOP (EE 2.9 or later).
>  
>       We need to do this because of a hardware bug in the R5900 chip.  */
> -  if (mips_opts.arch == CPU_R5900
> +  if ((mips_opts.arch == CPU_R5900 || mips_fix_r5900)

 Please make `-mfix-r5900' independent from `-march=r5900', so that 
`-march=r5900 -mno-fix-r5900' (or `-mno-fix-r5900 -march=r5900') does not 
enable the workaround.  I'm happy with the default for `-march=r5900' to 
be `-mfix-r5900' of course, reflecting the current situation, but please 
document it in the manual too.

 The reason is we always want to be able to flip each option off, as 
documented anyway.

> diff --git a/gas/doc/as.texi b/gas/doc/as.texi
> index acecd35225..1270e6efa1 100644
> --- a/gas/doc/as.texi
> +++ b/gas/doc/as.texi
> @@ -1444,6 +1445,11 @@ of an mfhi or mflo instruction occurs in the following two instructions.
>  Cause nops to be inserted if a dmult or dmultu instruction is
>  followed by a load instruction.
>  
> +@item -mfix-r5900
> +@itemx -mno-fix-r5900
> +Insert a NOP in the branch delay slot for short loops with six
> +instructions or less.

 How about:

Do not attempt to schedule the preceding instruction into the delay slot 
of a branch instruction placed at the end of a short loop of six 
instructions or less and always schedule a @code{nop} instruction there
instead.

?  I think it will be more precise (also we're inconsistent WRT 
instruction markup, but I think @code fits best here; if in doubt check 
PDF output for visual quality).

> diff --git a/gas/doc/c-mips.texi b/gas/doc/c-mips.texi
> index 7751ce01d6..3ee407924c 100644
> --- a/gas/doc/c-mips.texi
> +++ b/gas/doc/c-mips.texi
> @@ -327,6 +327,11 @@ Insert nops to work around the 24K @samp{eret}/@samp{deret} errata.
>  Replace @code{pref} hints 0 - 4 and 6 - 24 with hint 28 to work around
>  certain CN63XXP1 errata.
>  
> +@item -mfix-r5900
> +@itemx -mno-fix-r5900
> +Insert a @samp{nop} in the branch delay slot for short loops with six
> +instructions or less.

 Likewise.

> diff --git a/gas/po/sv.po b/gas/po/sv.po
> index 575bf0621d..cee660b315 100644
> --- a/gas/po/sv.po
> +++ b/gas/po/sv.po
> @@ -12546,6 +12546,7 @@ msgid ""
>  "-mfix-vr4130\t\twork around VR4130 mflo/mfhi errata\n"
>  "-mfix-24k\t\tinsert a nop after ERET and DERET instructions\n"
>  "-mfix-cn63xxp1\t\twork around CN63XXP1 PREF errata\n"
> +"-mfix-r5900\t\twork around R5900 short loop errata\n"
>  "-mgp32\t\t\tuse 32-bit GPRs, regardless of the chosen ISA\n"
>  "-mfp32\t\t\tuse 32-bit FPRs, regardless of the chosen ISA\n"
>  "-msym32\t\t\tassume all symbols have 32-bit values\n"
> @@ -12560,6 +12561,7 @@ msgstr ""
>  "-mfix-vr4130\t\tlösning för VR4130 mflo/mfhi-errata\n"
>  "-mfix-24k\t\tinfoga en nop efter ERET- och DERET-instruktioner\n"
>  "-mfix-cn63xxp1\t\tlösning för CN63XXP1 PREF-errata\n"
> +"-mfix-r5900\t\tlösning för R5900 kort loop-errata\n"
>  "-mgp32\t\t\tanvänd 32-bitars GPRs, oberoende av vald ISA\n"
>  "-mfp32\t\t\tanvänd 32-bitars FPRs, oberoende av vald ISA\n"
>  "-msym32\t\t\tantag att alla symbole har 32-bitars värden\n"

 This stuff is normally done by the translation team shortly before a 
release, so I believe it will best be dropped at this stage.

> diff --git a/gas/testsuite/gas/mips/r5900-fix.s b/gas/testsuite/gas/mips/r5900-fix.s
> new file mode 100644
> index 0000000000..167eb2c69f
> --- /dev/null
> +++ b/gas/testsuite/gas/mips/r5900-fix.s
> @@ -0,0 +1,19 @@
> +	.text
> +
> +test_mfix_r5900:
> +	.ent test_mfix_r5900

 Please swap these two lines as `.ent' is supposed to precede the label 
referred (we may not actually enforce that, but let's keep code right).

> +	.set push
> +	.set reorder

 The `reorder' mode is the default, so you don't need them, and neither 
`.set pop' below.

> +	# Test the short loop fix with 3 loop instructions.
> +	li $3, 300
> +short_loop_mfix_r5900:
> +	addi $3, -1
> +	addi $4, -1
> +	# A NOP will be inserted in the branch delay slot.
> +	bne $3, $0, short_loop_mfix_r5900
> +
> +	li $4, 3
> +	.set pop
> +
> +	.space	8
> +	.end test_mfix_r5900

 Can you make this test verify boundary cases?  It would be more useful 
than picking a random size of a loop from the middle of the affected range 
(which of course you can keep regardless).  We want to make sure we get 
the range right, now and forever.

 Did you run regression-testing with your change?  How did you verify it?

  Maciej