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

[Richard Henderson]

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.


r~