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

[Michael Rolnik]

1. The code was generated. Every instruction has its own extractors. there
are several reasons
    1. I don't have to think and gather the instructions into the group.
    2. I don't have to remember what group an instruction is in
    3. there is no performance penalty.

2. done.


On Sun, Jun 5, 2016 at 1:17 AM, Richard Henderson <address@hidden> wrote:

> 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~
>



-- 
Best Regards,
Michael Rolnik