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

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

Hi Fredrik,

 Thank you for your submission.  Your change looks very good to me, except 
for a bunch of minor nits in your proposed ChangeLog entry.

> 	* gcc/config/mips/mips.c (mips_reorg_process_insns)
> 	  (mips_option_override): Default to working around R5900
> 	  errata only if the processor was selected explicitly.

 I think this only describes the `mips_option_override' part.  ChangeLog 
entries are best concise, so how about just:

	* config/mips/mips.c (mips_reorg_process_insns)
	(mips_option_override): Handle `-mfix-r5900'.

?

> 	* gcc/config/mips/mips.h: Declare `mfix-r5900' and
> 	  `mno-fix-r5900'.

 This has to be:

	* config/mips/mips.h (ASM_SPEC): Add `mfix-r5900' and 
	`mno-fix-r5900'.

as you're adding to the ASM_SPEC definition.  Also your patch lines are 
way off and I had to seach for the place this change applies to -- have 
you been using current upstream trunk with this change?

> 	* gcc/config/mips/mips.opt: Define MASK_FIX_R5900.

 Likewise:

	* config/mips/mips.opt (mfix-r5900): New option.

as it's an entry for `mfix-r5900' that you're adding.

> 	* gcc/doc/invoke.texi: Document the R5900, `mfix-r5900' and
> 	  `mno-fix-r5900'.

 It looks like missing bits, did you mean:

	* doc/invoke.texi: Document the `r5900' processor name, and 
	`-mfix-r5900' and `-mno-fix-r5900' options.

?

 All these entries apply to the gcc/ChangeLog rather than the top-level 
one, so please drop the leading gcc/ part of the paths, and indicate it at 
the beginning instead.

 Please also fix indentation throughout; the subsequent lines of entries 
are horizontally aligned with the asterisk above and not indented any 
further.

 Please resubmit your change with the ChangeLog entry updated.  
Regrettably I have no authority to approve your submission, but hopefully 
a maintainer will so that it can make it to GCC 9.

  Maciej