Re: [Fwd: Re: [PATCH] MIPS32 DSP instructions again]

[Eric Christopher <echristo at redhat dot com>]

Hiya,

> +       case '3': USE_BITS
> (OP_MASK_SA3,                OP_SH_SA3);     break;
> +       case '4': USE_BITS
> (OP_MASK_SA4,                OP_SH_SA4);     break;
> +       case '5': USE_BITS (OP_MASK_IMM8,               OP_SH_IMM8);
> break;
> 

Spacing here.

> +             case '7': /* four dsp accumulators in bits 11,12 */ 
> +             if (s[0] == '$' && s[1] == 'a' && s[2] == 'c' &&
> +                 s[3] >= '0' && s[3] <= '3')
> 

Can you change this to be $acc instead of $ac? It'll match with other
targets better.

> +             case '9': /* four dsp accumulators in bits 21,22 */
> +             if (s[0] == '$' && s[1] == 'a' && s[2] == 'c' &&
> +                 s[3] >= '0' && s[3] <= '3')
> 

Ditto.

> +           case '0': /* dsp 6-bit signed immediate in bit 20 */
> +             my_getExpression (&imm_expr, s);
> +             check_absolute_expr (ip, &imm_expr);
> +             if ((imm_expr.X_add_number + ((OP_MASK_DSPSFT + 1) >>
> 1)) &
> +                 ~OP_MASK_DSPSFT)

Can you rewrite this to make the ranges more clear?


> +           case ':': /* dsp 7-bit signed immediate in bit 19 */
> +             my_getExpression (&imm_expr, s);
> +             check_absolute_expr (ip, &imm_expr);
> +             if ((imm_expr.X_add_number + ((OP_MASK_DSPSFT_7 + 1) >>
> 1)) &
> +                 ~OP_MASK_DSPSFT_7)

Ditto. Really for most of the range checks that you do in this patch.

> *** 8172,8179 ****
>             case 'c':           /* break code */
>               my_getExpression (&imm_expr, s);
>               check_absolute_expr (ip, &imm_expr);
> !             if ((unsigned long) imm_expr.X_add_number > 1023)
> !               as_warn (_("Illegal break code (%lu)"),
>                          (unsigned long) imm_expr.X_add_number);
>               INSERT_OPERAND (CODE, *ip, imm_expr.X_add_number);
>               imm_expr.X_op = O_absent;
> --- 8340,8348 ----
>             case 'c':           /* break code */
>               my_getExpression (&imm_expr, s);
>               check_absolute_expr (ip, &imm_expr);
> !             if (imm_expr.X_add_number & ~OP_MASK_CODE)
> !               as_warn (_("Immediate for %s not in range 0..%d (%
> lu)"),
> !                        ip->insn_mo->name, OP_MASK_CODE,
>                          (unsigned long) imm_expr.X_add_number);
>               INSERT_OPERAND (CODE, *ip, imm_expr.X_add_number);
>               imm_expr.X_op = O_absent;

Why?

> + #if 0 /* XXX FIXME */
> +   if (file_ase_dsp)
> +     elf_elfheader (stdoutput)->e_flags |= ???;
> + #endif
> 

Just leave this out. A large block comment above the header flag setting
code that says why we aren't putting some in there and what we need to
include would be useful.


> + #define WR_a  WR_HILO /* Write dsp accumulators (reuse WR_HILO)  */
> + #define RD_a  RD_HILO /* Read dsp accumulators (reuse RD_HILO)  */

Hmm? Why are we reusing this? Especially since you're adding another
mt/mfhi mt/mflo.

-eric