Re: [PATCH v2] 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 update.  It goes in the right direction, but there is 
still a nit I would like you to address.

> `-march=r5900' already enables the R5900 short loop workaround.
> However, the R5900 ISA and most other MIPS ISAs are mutually
> exclusive since R5900-specific instructions are generated as well.
> 
> The `-mfix-r5900' option can be used in combination with e.g.
> `-mips2' or `-mips3' to generate generic MIPS binaries that also
> work with the R5900 target.
> 
> This change has been tested with `make RUNTESTFLAGS=mips.exp
> check-gas' for the targets `mipsr5900el-unknown-linux-gnu',
> `mipsr5900el-elf' and `mips3-unknown-linux-gnu'.

 Please prepare a suitable ChangeLog entry.  I have missed its absence in 
the initial review somehow (sorry about that), but since we need another 
iteration anyway, please include it alongside.

> @@ -6997,7 +7005,8 @@ 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_fix_r5900_explicit ?
> +       mips_fix_r5900 : mips_opts.arch == CPU_R5900)

 The GNU coding standard requires operators to be included at the start of 
the continuation line rather than at the end of one being wrapped, so this 
would have to be rewritten as:

  if ((mips_fix_r5900_explicit
       ? mips_fix_r5900 : mips_opts.arch == CPU_R5900)

or:

  if ((mips_fix_r5900_explicit ? mips_fix_r5900
       : mips_opts.arch == CPU_R5900)

or indeed:

  if ((mips_fix_r5900_explicit ? mips_fix_r5900 : mips_opts.arch == CPU_R5900)

as this fits in the 79 column limit.  However please set `mips_fix_r5900' 
appropriately in `mips_after_parse_args' instead, i.e.:

  if (!mips_fix_r5900_explicit)
    mips_fix_r5900 = file_mips_opts.arch == CPU_R5900;

so that it's the only setting referred in determination as to whether to 
enable the workaround.  This will affect temporary `.set arch=' overrides, 
but I think they are not supposed to override the global `-mfix-r5900' 
setting, whether specified or inferred.

 This is otherwise OK, so please resubmit with just this issue addressed.

  Maciej