This is the mail archive of the
binutils@sources.redhat.com
mailing list for the binutils project.
Re: [Fwd: Re: [PATCH] MIPS32 DSP instructions again]
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