Re: [Qemu-devel] [PATCH v2] target-mips: Implement Loongson Multimedia Instructions

[Aurelien Jarno <aurelien@aurel32.net>]

On Mon, Sep 17, 2012 at 11:54:59AM -0700, Richard Henderson wrote:
> On 09/07/2012 05:32 PM, Aurelien Jarno wrote:
> >> +    do_shift:
> >> +        switch (opc) {
> >> +        case OPC_SLL_CP2:
> >> +        case OPC_DSLL_CP2:
> >> +            tcg_gen_shl_i64(t0, t0, t1);
> >> +            break;
> >> +        case OPC_SRA_CP2:
> >> +        case OPC_DSRA_CP2:
> >> +            /* Since SRA is UndefinedResult without sign-extended inputs,
> >> +               we can treat SRA and DSRA the same.  */
> >> +            tcg_gen_sar_i64(t0, t0, t1);
> >> +            break;
> >> +        case OPC_SRL_CP2:
> >> +            /* We want to shift in zeros for SRL; zero-extend first.  */
> >> +            tcg_gen_ext32u_i64(t0, t0);
> >> +            /* FALLTHRU */
> >> +        case OPC_DSRL_CP2:
> >> +            tcg_gen_shr_i64(t0, t0, t1);
> >> +            break;
> >> +        }
> > 
> > You probably want to and t1 with 0x3f, to make sure to not have a shift
> > larger then 64.
> 
> Done.  Though as discussed elsewhere today I think we ought to make this
> merely undefined results as opposed to undefined behaviour in TCG.
> 
> >> +        /* Shifts larger than MAX produce zero.  */
> >> +        tcg_gen_setcondi_i64(TCG_COND_LTU, t1, t1, shift_max);
> >> +        tcg_gen_neg_i64(t1, t1);
> > 
> > I guess you want tcg_gen_subi_i64(t1, t1, 1);
> 
> No.  You're confusing the computations of
> 
>   (x >= 32) - 1
> and
>   -(x < 32)
> 
> Logically the same results but the computation is different.
> And the later will of course be smaller on i386 due to neg insn.
> 
> >> +    case OPC_ADD_CP2:
> >> +    case OPC_DADD_CP2:
> >> +        {
> >> +            /* Since ADD is UndefinedResult without sign-extended inputs,
> >> +               we can treat both ADD and DADD the same.  */
> > 
> > I don't think this is correct. For ADD, the result has to be signed
> > extended. Also the exception condition is not the same for ADD and DADD.
> 
> Fixed the sign extension here and SUB.
> 
> The exception condition *is* the same, after the sign extension is done.
> Please go back and compare the code in the existing ADD/DADD expansion.

Yes, I agree it is the same once sign extended, I agree. What I meant is
that for the same input values you might have an exception on ADD and
not on DADD.


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net