Re: [Qemu-devel] [PATCH for-2.5 28/30] m68k: shift/rotate bytes and words

[Richard Henderson <rth@twiddle.net>]

On 08/09/2015 01:13 PM, Laurent Vivier wrote:
> +#define HELPER_SHL(type, bits) \
> +uint32_t HELPER(glue(glue(shl, bits), _cc))(CPUM68KState *env, \
> +                                            uint32_t val, uint32_t shift) \
> +{ \
> +    type result; \
> +    uint32_t cf; \
> +    shift &= 63; \
> +    if (shift == 0) { \
> +        result = (type)val; \
> +        cf = 0; \
> +    } else if (shift < bits) { \
> +        result = (type)val << shift; \
> +        cf = ((type)val >> (bits - shift)) & 1; \
> +    } else if (shift == bits) { \
> +        result = 0; \
> +        cf = val & 1; \
> +    } else { \
> +        result = 0; \
> +        cf = 0; \
> +    } \

Perhaps this would be cleaner to simply operate on a 64-bit type.

	uint64_t res = (type)val;
        res <<= shift & 63;
	cf = (res >> bits) & 1;

For shift == 0, we've not set bit BITS, so it's zero.
For shift <= BITS, we've obviously got the correct data.
For shift > BITS, we've shifted val all the way past and again have zero.

> +#define HELPER_SHR(type, bits) \
> +uint32_t HELPER(glue(glue(shr, bits), _cc))(CPUM68KState *env, \
> +                                            uint32_t val, uint32_t shift) \
> +{ \
> +    type result; \
> +    uint32_t cf; \
> +    shift &= 63; \
> +    if (shift == 0) { \
> +        result = (type)val; \
> +        cf = 0; \
> +    } else if (shift < bits) { \
> +        result = (type)val >> shift; \
> +        cf = ((type)val >> (shift - 1)) & 1; \
> +    } else if (shift == bits) { \
> +        result = 0; \
> +        cf = (type)val >> (bits - 1); \
> +    } else { \
> +        result = 0; \
> +        cf = 0; \
> +    } \

Similarly.

> +#define HELPER_SAL(type, bits) \
> +uint32_t HELPER(glue(glue(sal, bits), _cc))(CPUM68KState *env, \
> +                                            uint32_t val, uint32_t shift) \
> +{ \
> +    type result; \
> +    uint32_t cf; \
> +    uint32_t vf; \
> +    uint32_t m; \
> +    shift &= 63; \
> +    if (shift == 0) { \
> +        vf = 0; \
> +    } else if (shift < bits) { \
> +        m = ((1llu << (shift + 1)) - 1) << (bits - shift - 1); \
> +        vf = (val & m) != m && (val & m) != 0; \
> +    } else { \
> +        m = (1llu << bits) - 1; \
> +        vf = (val & m) != 0; \
> +    } \

This computation of VF seems overly complex.  It's just

  (type)(val ^ result) < 0

for all cases.

> +DISAS_INSN(shift8_im)
> +DISAS_INSN(shift16_im)
>  DISAS_INSN(shift_im)
...
> +DISAS_INSN(shift8_reg)
> +DISAS_INSN(shift16_reg)
>  DISAS_INSN(shift_reg)
...
> +DISAS_INSN(shift_mem)

Surely some code could be shared here...


r~