Re: [PATCH MIPS][LS3A] Add Loongson3A mul/div instructions

[Richard Sandiford <rdsandiford at googlemail dot com>]

Mingming Sun <mingm.sun@gmail.com> writes:
> 2010-11-29 Mingming Sun <mingm.sun@gmail.com>
> 	gas/ChangeLog
> 	* config/tc-mips.c (macro_build): Add loongson3a-specific instructions support.
> 	(validate_mips_insn): Likewise.
> 	(mips_ip): Likewise.
>
> 	include/ChangeLog
> 	* opcode/mips.h: Define OP_SH_MINUS_A, OP_MASK_MINUS_A, OP_SH_MINUS_B,
> 	OP_MASK_MINUS_B, OP_SH_MINUS_F, OP_MASK_MINUS_F, OP_SH_RY, OP_MASK_RY,
> 	OP_SH_FZ, OP_MASK_FZ, INSN_WRITE_GPR_Y, INSN_WRITE_FPR_Z,
> 	INSN_READ_GPR_D, INSN_READ_GPR_Y, INSN_READ_FPR_Z.
>
> 	opcodes/ChangeLog
> 	* mips-dis.c (print_insn_args): Add loongson3a-specific instructions support.
> 	* mips-opc.c: Define WR_z, RD_y, RD_z, RD_d.
> 	(mips_builtin_opcodes): Add loongson3a-specific instructions.

There's a big comment in include/opcode/mips.h that explains what
the format characters mean, and which ones have been used.  You need
to update that as well.

Is there any need for a new extension character, "-"?  Many characters
in the "+" group are still free, and they aren't set aside for a
particular purpose.  I'd also prefer "+Z" over "^" for the FZ fields.

> @@ -430,6 +442,12 @@ struct mips_opcode
>  #define INSN_WRITE_FPR_S            0x00000010
>  /* Modifies the floating point register in OP_*_FT.  */
>  #define INSN_WRITE_FPR_T            0x00000020
> +/* Loongson-3A: Modifies the general purpose register in OP_*_RY.  */
> +#define INSN_WRITE_GPR_Y            0x00000009
> +/* Loongson-3A: Modifies the floating point register in OP_*_FZ.  */
> +#define INSN_WRITE_FPR_Z            0x00000090
> +/* Loongson-3A: Reads the general purpose register in OP_*_RD.  */
> +#define INSN_READ_GPR_D             0x0000000c
>  /* Reads the general purpose register in OP_*_RS.  */
>  #define INSN_READ_GPR_S             0x00000040
>  /* Reads the general purpose register in OP_*_RT.  */
> @@ -440,6 +458,11 @@ struct mips_opcode
>  #define INSN_READ_FPR_T             0x00000200
>  /* Reads the floating point register in OP_*_FR.  */
>  #define INSN_READ_FPR_R		    0x00000400
> +/* Loongson-3A: Reads the general purpose register in OP_*_RY.  */
> +#define INSN_READ_GPR_Y             0x00000030
> +/* Loongson-3A: Reads the floating point register in OP_*_FZ.  */
> +#define INSN_READ_FPR_Z             0x00000900

These INSN_*s values are supposed to be bitflags, so (to take an example),
0x900 already means "reads the FPR in the FS field and modifies the
condition codes".  You should be using new INSN2_* flags instead.

You should also be checking these flags in tc-mips.c, in the same
way that existing read and write flags are set.  This controls things
like automatic delay-slot filling.  For example, at the moment:

	gslq $4,$5,0($6)
        beq  $4,$0,1f
        nop
1:

produces:

   0:   10800002        beqz    a0,0xc
   4:   c8c50024        gslq    a0,a1,0(a2)
   8:   00000000        nop

which looks wrong.

There needs to be some gas testcases too.

Richard