Re: [Qemu-devel] [PATCH v5 2/8] target/mips: Support R5900 specific three-operand MULT and MULTU

[Philippe Mathieu-Daudé]

On Fri, Sep 28, 2018 at 5:16 PM Fredrik Noring <address@hidden> wrote:
>
> Hi Philippe,
>
> > Can you copy/paste some info regarding those instructions from the ISA
> > here, to note how they differ? ...
>
> Yes. Other corresponding functions typically do not seem to have such ISA
> notes, but I can certainly write one for it.

My guess is they don't have because they implement the generic ISA
which is public.

It is rather hard to find public manuals for the Toshiba/Sony ISA.

> > Since we have acc = 0 we can directly use cpu_LO[0] and cpu_HI[0],
> > removing needs for an 'acc' argument.
>
> We could, but as Maciej replied there are pipeline 1 versions of these
> instructions which would have acc = 1. I retained the acc parameter mainly
> to avoid the error of forgetting to replace 0 with acc when introducing
> them later on.

Yes, I was not aware of those instructions, so better keep the 'acc' argument.

> > > +{
> > > +    TCGv t0 = tcg_temp_new();
> > > +    TCGv t1 = tcg_temp_new();
> > > +
> > > +    gen_load_gpr(t0, rs);
> > > +    gen_load_gpr(t1, rt);
> > > +
> > > +    switch (opc) {
> > > +    case OPC_MULT:
> > > +        {
> > > +            TCGv_i32 t2 = tcg_temp_new_i32();
> > > +            TCGv_i32 t3 = tcg_temp_new_i32();
> > > +            tcg_gen_trunc_tl_i32(t2, t0);
> > > +            tcg_gen_trunc_tl_i32(t3, t1);
> > > +            tcg_gen_muls2_i32(t2, t3, t2, t3);
> > > +            if (rd)
> >
> > Check QEMU CODING_STYLE "Block structure":
> >
> >   Every indented statement is braced; even if the block contains
> >   just one statement.  The opening brace is on the line that
> >   contains the control flow statement that introduces the new block;
> >   the closing brace is on the same line as the else keyword, or on
> >   a line by itself if there is no else keyword.
> >
> > QEMU scripts/checkpatch.pl reports such mistakes.
>
> Done.
>
> > > +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
> >
> > I'd use:
> >
> >                    gen_move_low32(cpu_gpr[rd], t2);
>
> Are you sure this is correct? The GPR rd must be sign-extended.

Richard can you verify please?

> > > +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
> >
> > So:
> >
> >                tcg_gen_ext_i32_tl(cpu_LO[0], t2);
> >
> > > +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
> >
> > And:
> >
> >                tcg_gen_ext_i32_tl(cpu_HI[0], t3);
>
> Done.
>
> > > +            tcg_temp_free_i32(t2);
> > > +            tcg_temp_free_i32(t3);
> > > +        }
> > > +        break;
> > > +    case OPC_MULTU:
> > > +        {
> > > +            TCGv_i32 t2 = tcg_temp_new_i32();
> > > +            TCGv_i32 t3 = tcg_temp_new_i32();
> > > +            tcg_gen_trunc_tl_i32(t2, t0);
> > > +            tcg_gen_trunc_tl_i32(t3, t1);
> > > +            tcg_gen_mulu2_i32(t2, t3, t2, t3);
> > > +            if (rd)
> > > +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
> > > +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
> > > +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
> >
> > Same comments from MULT apply here.
>
> Done.
>
> > > +            tcg_temp_free_i32(t2);
> > > +            tcg_temp_free_i32(t3);
> > > +        }
> > > +        break;
> > > +    default:
> > > +        MIPS_INVAL("mul R5900");
> > > +        generate_exception_end(ctx, EXCP_RI);
> > > +        goto out;
> > > +    }
> > > +
> > > + out:
> > > +    tcg_temp_free(t0);
> > > +    tcg_temp_free(t1);
> > > +}
> > > +
> > >  static void gen_mul_vr54xx (DisasContext *ctx, uint32_t opc,
> > >                              int rd, int rs, int rt)
> > >  {
> > > @@ -22378,6 +22429,8 @@ static void 
> > > decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx)
> > >              check_insn(ctx, INSN_VR54XX);
> > >              op1 = MASK_MUL_VR54XX(ctx->opcode);
> > >              gen_mul_vr54xx(ctx, op1, rd, rs, rt);
> > > +        } else if (ctx->insn_flags & INSN_R5900) {
> > > +            gen_mul_r5900(ctx, op1, 0, rd, rs, rt);
> >
> > Removing 'acc' arg:
> >
> >                gen_mul_r5900(ctx, op1, rd, rs, rt);
>
> Done.
>
> > Note, these instructions are also valid on the R3900 (which also has
> > MADD/MADDU).
> >
> > Would gen_mul_toshiba() be a better common name? I don't like it but
> > can't think of another.
>
> I propose gen_mul_3op, since its distinctive feature is the three operands.

Fine by me.