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

[Laurent Vivier]

Le 09/11/2016 à 20:39, Richard Henderson a écrit :
> On 11/09/2016 07:47 PM, Richard Henderson wrote:
>> On 11/09/2016 06:30 PM, Laurent Vivier wrote:
>>> +        /* create [src:X:..] */
>>> +
>>> +        tcg_gen_deposit_i32(t0, QREG_CC_X, src, 1, size);
>>> +        tcg_gen_shli_i32(t0, t0, 31 - size);
>>> +
>>> +        /* rotate */
>>> +
>>> +        tcg_gen_rotl_i32(t0, t0, shift);
>>> +
>>> +        /* result is [src:..:src:X] */
>>> +
>>> +        tcg_gen_andi_i32(X, t0, 1);
>>> +        tcg_gen_shri_i32(t0, t0, 1);
>>
>> I don't see how this is supposed to work.  If you form [src:x:...],
>> and rotate
>> by 0, then X gets garbage.  Of course, you're actually forming

The result is ignored in the case of a rotate by 0 (see movcond in
rotate_reg()).

>> [0:src:x].  But
>> for a rol of 2, the lsb of src gets 0's instead of the msb of src.

for a rol of 2 on an 8bit value [12345678]:

    tcg_gen_deposit_i32(t0, QREG_CC_X, src, 1, size);
    tcg_gen_shli_i32(t0, t0, 31 - size);

t0 = [12345678x00000000000000000000000]

    tcg_gen_rotl_i32(t0, t0, shift);

t0 = [345678x0000000000000000000000012]

    tcg_gen_andi_i32(X, t0, 1);

X = 2

    tcg_gen_shri_i32(t0, t0, 1);

t0 = [0345678x000000000000000000000001]

    tcg_gen_shri_i32(t1, t0, 31 - size);

t1 = [000000000000000000000000345678x0]

    tcg_gen_or_i32(dest, t0, t1);

dest = [0345678x0000000000000000345678x1]

->     we keep only 8 bits: [345678x1]

Where am I wrong?

> Feh, actually that only works for rot8, since you need 2*size+1 bits
> available for this.  Why not just use shifts.  Something like
> 
>   int shl = shift % 17;
>   int shr = shl ^ (size - 1);
>   int shx = shl ? shl - 1 : 16;
> 
>   res32 = (data << shl) | (data >> shr) | (x << shx);
>   x = (res32 >> 16) & 1;


If you think it is better, I can do like that.

Thanks,
Laurent