RE: [PATCH] Add support for MIPS DSPr3

["Maciej W. Rozycki" <macro at imgtec dot com>]

On Tue, 10 May 2016, Matthew Fortune wrote:

> >  Thank you for your contribution.  Your change looks good overall, except
> > for a minor nit, see below.  However it adds new command-line options, so
> > a corresponding GAS manual update is required.  Please resubmit with these
> > changes made.
> 
> Thanks, docs for command line and directives added. 

 Thanks, please update gas/doc/as.texinfo too, both the option summary and 
option details.  You can reuse the relevant part of gas/doc/c-mips.texi 
for the latter.  Remember to adjust the ChangeLog entry accordingly.

> gas/
> 
> 	* config/tc-mips.c (enum options): Add OPTION_DSPR3 and
> 	OPTION_NO_DSPR3.

 Please use just (options) here, we only use the entity's name in 
ChangeLog entries.

> 	(mips_convert_ase_flags): Map ASE_DSPR6 to AFL_ASE_DSPR3.

 Typo here s/ASE_DSPR6/ASE_DSPR3/.

 You missed the change to `md_show_usage'.

> opcodes/
> 
> 	* mips-dis.c (mips_arch_choices): Add ASE_DSPR3 to MIPS32r6 and
> 	mips64r6.

 s/MIPS32r6/mips32r6/

> 	* mips-opc.c (D37): New macro.

 s/D37/D34/

 Sorry to miss the ChangeLog bits, I inadvertently skipped the entry 
entirely in the first pass.  A couple more nits below.

> diff --git a/gas/doc/c-mips.texi b/gas/doc/c-mips.texi
> index 30544db..0b16e08 100644
> --- a/gas/doc/c-mips.texi
> +++ b/gas/doc/c-mips.texi
> @@ -191,6 +191,13 @@ This option implies -mdsp.
>  This tells the assembler to accept DSP Release 2 instructions.
>  @samp{-mno-dspr2} turns off this option.
>  
> +@item -mdspr3
> +@itemx -mno-dspr3
> +Generate code for the DSP Release 3 Application Specific Extension.
> +This option implies -mdsp and -mdspr2.

 Please use @samp{} here too (and then in gas/doc/as.texinfo of course).

> diff --git a/gas/testsuite/gas/mips/mips32-dspr3.s b/gas/testsuite/gas/mips/mips32-dspr3.s
> new file mode 100644
> index 0000000..519478d
> --- /dev/null
> +++ b/gas/testsuite/gas/mips/mips32-dspr3.s
> @@ -0,0 +1,12 @@
> +# source file to test assembly of MIPS DSP ASE Rev3 for MIPS32 instructions
> +
> +	.set noreorder
> +	.set noat
> +
> +	.text
> +text_label:
> +	bposge32c text_label
> +
> +# Force at least 8 (non-delay-slot) zero bytes, to make 'objdump' print ...
> +	.align	2
> +	.space	8

 You still have a space with `.set' directives and a tab with `.align' and 
`.space'.  I can see other files are inconsistent in the same way though 
and it's a minor issue, so I won't persist on this occasion.

 OK to apply with these updates, thanks for your work.

  Maciej