Re: [Qemu-devel] [PATCH RFC v20 4/8] target-avr: Add instruction decoding

[Richard Henderson <rth@twiddle.net>]

On 6/3/19 3:13 PM, Michael Rolnik wrote:
> Richard,
> I don't understand what I should do. Do you want me to merge decode files?

Yes, using the mechanisms previously described.


r~

> 
> On Fri, May 31, 2019 at 5:45 PM Richard Henderson <richard.henderson@linaro.org
> <mailto:richard.henderson@linaro.org>> wrote:
> 
>     On 5/30/19 2:07 PM, Michael Rolnik wrote:
>     > This includes:
>     > - encoding of all 16 bit instructions
>     > - encoding of all 32 bit instructions
>     >
>     > Signed-off-by: Michael Rolnik <mrolnik@gmail.com <mailto:mrolnik@gmail.com>>
>     > ---
>     >  target/avr/insn16.decode | 160 +++++++++++++++++++++++++++++++++++++++
>     >  target/avr/insn32.decode |  10 +++
>     >  2 files changed, 170 insertions(+)
>     >  create mode 100644 target/avr/insn16.decode
>     >  create mode 100644 target/avr/insn32.decode
> 
>     Two things:
> 
>     (1) decodetree can handle variable-width ISA now.
> 
>     It's slightly ugly in that the %field numbering is little-endian and thus
>     varies for each insn size.  But the in-flight patch set for target/rx shows
>     that it works.
> 
>     That said, I don't think you need that because,
> 
>     (2) The four instructions that are 32-bits do not have
>         any opcode bits in the second 16-bits.
> 
>     E.g.
> 
>     # The 22-bit immediate is partially in the opcode word,
>     # and partially in the next.  Use append_16 to build the
>     # complete 22-bit value.
>     %imm_call       4:5 0:1                 !function=append_16
>     CALL            1001 010. .... 111.     imm=%imm_call
>     JMP             1001 010. .... 110.     imm=%imm_call
> 
>     # The 16-bit immediate is completely in the next word.
>     # Fields cannot be defined with no bits, so we cannot play
>     # the same trick and append to a zero-bit value.
>     # Defer reading the immediate until trans_{LDS,STS}.
>     @ldst_s         .... ... rd:5 ....      imm=0
>     LDS             1001 000 ..... 0000     @ldst_s
>     STS             1001 001 ..... 0000     @ldst_s
> 
> 
>     static uint16_t next_word(DisasContext *ctx)
>     {
>         return cpu_lduw_code(ctx->env, ctx->npc++ * 2);
>     }
> 
>     static int append_16(DisasContext *ctx, int x)
>     {
>         return x << 16 | next_word(ctx);
>     }
> 
>     static bool trans_LDS(DisasContext *ctx, arg_ldst_s *a)
>     {
>         a->imm = next_word(ctx);
>         // other stuff
>     }
> 
>     I realize that next_word as written does not fit in to how you currently
>     process instructions in the loop, but I also think that's a mistake.  I'll
>     respond to that in its place in the next patch.
> 
>     That said, next_word *could* be written to use ctx->inst[0].opcode.
> 
> 
>     r~
> 
> 
> 
> -- 
> Best Regards,
> Michael Rolnik