Re: [Qemu-devel] [PATCH 04/10] target-avr: adding instructions encodings

[Richard Henderson]

On 06/02/2016 01:06 PM, Michael Rolnik wrote:
> Signed-off-by: Michael Rolnik <address@hidden>
> ---
>  target-avr/translate-inst.h | 838 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 838 insertions(+)
>  create mode 100644 target-avr/translate-inst.h
>
> diff --git a/target-avr/translate-inst.h b/target-avr/translate-inst.h
> new file mode 100644
> index 0000000..6bd180d
> --- /dev/null
> +++ b/target-avr/translate-inst.h
> @@ -0,0 +1,838 @@
> +
> +#ifndef AVR_TRANSLATE_INST_H_
> +#define AVR_TRANSLATE_INST_H_
> +
> +typedef struct DisasContext    DisasContext;
> +
> +int     avr_translate_NOP(CPUAVRState *env, DisasContext* ctx, uint32_t 
> opcode);
> +
> +int     avr_translate_MOVW(CPUAVRState *env, DisasContext* ctx, uint32_t 
> opcode);
> +static inline uint32_t MOVW_Rr(uint32_t opcode)
> +{
> +    return extract32(opcode, 0, 4);
> +}
> +static inline uint32_t MOVW_Rd(uint32_t opcode)
> +{
> +    return extract32(opcode, 4, 4);
> +}
> +
> +int     avr_translate_MULS(CPUAVRState *env, DisasContext* ctx, uint32_t 
> opcode);
> +static inline uint32_t MULS_Rr(uint32_t opcode)
> +{
> +    return extract32(opcode, 0, 4);
> +}
> +static inline uint32_t MULS_Rd(uint32_t opcode)
> +{
> +    return extract32(opcode, 4, 4);
> +}

Any particular reason you're replicating all this logic for every instruction, 
as opposed to finding some commonalty between them?  There aren't *that* many 
different operand field types.

Glancing through the binutils source, I count 13.  Although I might have missed 
a handful, it's still an order of magnitude less than your 159 functions.

> +static inline uint32_t CPC_lRr(uint32_t opcode)
> +{
> +    return extract32(opcode, 0, 4);
> +}
...
> +static inline uint32_t CPC_hRr(uint32_t opcode)
> +{
> +    return extract32(opcode, 9, 1);
> +}

Any reason you're not simplifying the logic elsewhere by extracting the logical 
value, as opposed to the parts of the split value.  I.e.

    (extract32(opcode, 9, 1) << 4) | extract32(opcode, 0, 4)


r~