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

["Chao-ying Fu" <fu at mips dot com>]

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
>
>