Re: [Qemu-devel] [PATCH v2] target-m68k: add rol/ror/roxl/roxr instructions

[Laurent Vivier]

Le 11/11/2016 à 08:31, Richard Henderson a écrit :
> On 11/10/2016 11:51 PM, Laurent Vivier wrote:
>> +/* Result of rotate_x() is valid if 0 < shift < (size + 1) < 32 */
>> +static TCGv rotate_x(TCGv dest, TCGv src, TCGv shift, int left, int
>> size)
>> +{
>> +    TCGv X, shl, shr, shx;
>> +
>> +    shr = tcg_temp_new();
>> +    shl = tcg_temp_new();
>> +    shx = tcg_temp_new();
>> +    if (left) {
>> +        tcg_gen_mov_i32(shl, shift);      /* shl = shift */
>> +        tcg_gen_movi_i32(shr, size + 1);
>> +        tcg_gen_sub_i32(shr, shr, shift); /* shr = size + 1 - shift */
>> +        tcg_gen_subi_i32(shx, shift, 1);  /* shx = shift - 1 */
> 
> You're failing to bound shx properly.  If shift == 0, you produce -1
> here which is illegal.  You could mask this to 31, but it's even better

Yes, but in this case the result is ignored by the following movcond.

tcg/README says:

* shl_i32/i64 t0, t1, t2

t0=t1 << t2. Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64)

* shr_i32/i64 t0, t1, t2

t0=t1 >> t2 (unsigned). Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64)

An operation with "unspecified behavior" shall not crash.  However,
the result may be one of several possibilities so may be considered
an "undefined result".

> to produce size, as I said before, because then you don't have to have

Perhaps I've missed something (again), but I've tested with
"int shr = shl ^ (size - 1);" and it produces the wrong result.

for instance size = 8 and shift = 1

shr = 1 ^ 7 = 6

should be: 8 + 1 - 1 = 8

> 
>> +        tcg_gen_movcond_i32(TCG_COND_EQ, X,
>> +                            t1, zero,
>> +                            QREG_CC_X, X);
>> +        tcg_gen_movcond_i32(TCG_COND_EQ, DREG(insn, 0),
>> +                            t1, zero,
>> +                            DREG(insn, 0), res);
> 
> this at the end.  A zero shift will simply leave everything where it
> belongs, and you extract all the parts as normal.

To do "int shx = shl ? shl - 1 : 16;", we need a "movcond" in every
case, which should not be needed in the case of "rotate_im" where shift
is between 1 and 8.

> 
>> +DISAS_INSN(rotate_mem)
>> +{
>> +    TCGv src;
>> +    TCGv addr;
>> +    TCGv shift;
>> +    int left = (insn & 0x100);
>> +
>> +    SRC_EA(env, src, OS_WORD, 0, &addr);
>> +
>> +    shift = tcg_const_i32(1);
>> +    if (insn & 8) {
> 
> It's bit 9 for memory rotate.

right. I fix that.

Thanks,
Laurent