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]
Hello,
1. I didn't see any new line after case '5' from my patch.
2. Nigel has responded to the $acc issue.
3. For checking the immediate range, my original code is
as follows.
case ':': /* dsp 7-bit signed immediate in bit 19 */
my_getExpression (&imm_expr, s);
check_absolute_expr (ip, &imm_expr);
if ((unsigned long) (imm_expr.X_add_number + 64) > 127)
{
as_warn (_("DSP immediate not in range -64..63 (%ld)"),
(long) imm_expr.X_add_number);
}
But, I changed the magic number 64 to
((OP_MASK_DSPSFT_7 + 1) >> 1).
And, I changed the comparison of 127 to "AND" the inverse
of the mask. Do you prefer my original code or may I put
some comments to explain the code?
4. For case 'c', I change the warning message, because
our DSP instructions reuse this 'c' format. The old
warning message said "Illegal break code" which is not suitable
for DSP instructions.
5. Ok. I will put comments before the elf header flag setting.
6. WR_a/RD_a reuse WR_HILO/RD_HILO, because DSP accumulators
may map to the old HI/LO and we want to maintain correct
dependences of reading and writing HI/LO.
Thanks!
Regards,
Chao-ying
----- Original Message -----
From: "Eric Christopher" <echristo@redhat.com>
To: "Chao-ying Fu" <fu@mips.com>
Cc: "Thiemo Seufer" <ths@networkno.de>; <dom@mips.com>; <ian@airs.com>;
"Paul Koning" <pkoning@equallogic.com>; <nigel@mips.com>; "Richard
Sandiford" <rsandifo@nildram.co.uk>; <radhika@mips.com>;
<binutils@sourceware.org>
Sent: Monday, June 13, 2005 11:18 AM
Subject: 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
>
>