Re: [Qemu-devel] [PATCH for-2.5 29/30] m68k: add rol/rox/ror/roxr

[Richard Henderson <rth@twiddle.net>]

On 08/09/2015 01:13 PM, Laurent Vivier wrote:
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  target-m68k/helper.c    | 212 ++++++++++++++++++++++++++++++++++++++++++++++++
>  target-m68k/helper.h    |  14 ++++
>  target-m68k/translate.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 433 insertions(+)
> 
> diff --git a/target-m68k/helper.c b/target-m68k/helper.c
> index 16fca70..532f366 100644
> --- a/target-m68k/helper.c
> +++ b/target-m68k/helper.c
> @@ -25,6 +25,42 @@
>  
>  #define SIGNBIT (1u << 31)
>  
> +/* modulo 33 table */
> +const uint8_t rox32_table[64] = {
> +    0,  1,  2,  3,  4,  5,  6,  7,
> +    8,  9, 10, 11, 12, 13, 14, 15,
> +   16, 17, 18, 19, 20, 21, 22, 23,
> +   24, 25, 26, 27, 28, 29, 30, 31,
> +   32,  0,  1,  2,  3,  4,  5,  6,
> +    7,  8,  9, 10, 11, 12, 13, 14,
> +   15, 16, 17, 18, 19, 20, 21, 22,
> +   23, 24, 25, 26, 27, 28, 29, 30,
> +};
> +
> +/* modulo 17 table */
> +const uint8_t rox16_table[64] = {
> +    0,  1,  2,  3,  4,  5,  6,  7,
> +    8,  9, 10, 11, 12, 13, 14, 15,
> +   16,  0,  1,  2,  3,  4,  5,  6,
> +    7,  8,  9, 10, 11, 12, 13, 14,
> +   15, 16,  0,  1,  2,  3,  4,  5,
> +    6,  7,  8,  9, 10, 11, 12, 13,
> +   14, 15, 16,  0,  1, 2,   3,  4,
> +    5,  6,  7,  8,  9, 10, 11, 12,
> +};
> +
> +/* modulo 9 table */
> +const uint8_t rox8_table[64] = {
> +    0, 1, 2, 3, 4, 5, 6, 7,
> +    8, 0, 1, 2, 3, 4, 5, 6,
> +    7, 8, 0, 1, 2, 3, 4, 5,
> +    6, 7, 8, 0, 1, 2, 3, 4,
> +    5, 6, 7, 8, 0, 1, 2, 3,
> +    4, 5, 6, 7, 8, 0, 1, 2,
> +    3, 4, 5, 6, 7, 8, 0, 1,
> +    2, 3, 4, 5, 6, 7, 8, 0,
> +};

Why would you have these tables as opposed to just using the C modulo operator?

> +uint32_t HELPER(rol32)(uint32_t val, uint32_t shift)
> +{
> +    uint32_t result;
> +    if (shift == 0 || shift == 32) {
> +        return val;
> +    }
> +    result = (val << shift) | (val >> (32 - shift));
> +    return result;
> +}
> +
> +uint32_t HELPER(ror32)(uint32_t val, uint32_t shift)
> +{
> +    uint32_t result;
> +    if (shift == 0 || shift == 32) {
> +        return val;
> +    }
> +    result = (val >> shift) | (val << (32 - shift));
> +    return result;
> +}

Easily done in tcg directly.  But aren't these are actually unused?

> +#define HELPER_ROXR(type, bits) \
> +uint32_t HELPER(glue(glue(roxr, bits), _cc))(CPUM68KState *env, \
> +                                             uint32_t val, uint32_t shift) \
> +{ \

Again, I think perhaps a 64-bit shift type might help clean up these cases.
Start by forming the 34-bit quantity (X : VAL : 0); end by extracting bits
[32:1] as the result and bit 0 as X.

> +DISAS_INSN(rotate_im)
...
> +DISAS_INSN(rotate_reg)

Again, surely you can share code.


r~