Re: [PATCH v4 09/12] target/hexagon: import lexer for idef-parser

[Paolo Montesel <paolo.montesel.revng@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]

>
> > +/**
> > + * Semantic record of the IMM token, identifying an immediate constant
> > + */
> > +typedef struct HexImm {
> > +    union {
> > +        char id;            /**< Identifier of the immediate
>      */
> > +        uint64_t value;     /**< Immediate value (for VALUE type
> immediates) */
>
> Most immediates are 32 bits.  Since you treat them as 64 bits, you end up
> with unnecessary extends and truncates in the TCG.
>
> Here's an example from idef-generated-emitter.c
> void emit_J2_jump(DisasContext *ctx, Insn *insn, Packet *pkt, int riV)
> /* fIMMEXT(riV); (riV = riV & ~3); (PC = fREAD_PC()+riV);} */
> {
> int64_t qemu_tmp_0 = ~((int64_t)3ULL);
> int32_t qemu_tmp_1 = riV & qemu_tmp_0;
> riV = qemu_tmp_1;
> TCGv_i32 tmp_0 = tcg_temp_local_new_i32();
> tcg_gen_movi_i32(tmp_0, ctx->base.pc_next);
> TCGv_i64 tmp_1 = tcg_temp_local_new_i64();
> tcg_gen_ext_i32_i64(tmp_1, tmp_0);
>   <- Don't need this extension
> tcg_temp_free_i32(tmp_0);
> TCGv_i64 tmp_2 = tcg_temp_local_new_i64();
> tcg_gen_addi_i64(tmp_2, tmp_1, (int64_t)riV);                        <-
> This should be 32 bits
> tcg_temp_free_i64(tmp_1);
> TCGv_i32 tmp_3 = tcg_temp_local_new_i32();
> tcg_gen_trunc_i64_tl(tmp_3, tmp_2);
>  <- Don't need this truncation
> tcg_temp_free_i64(tmp_2);
> gen_write_new_pc(tmp_3);
> tcg_temp_free_i32(tmp_3);
> }
>

Thanks for spotting this. It's actually a bug in the lexer. The token
`{IMM_ID}"iV"` didn't initialize `bit_width`. Now it does. This is the
result:

void emit_J2_jump(DisasContext *ctx, Insn *insn, Packet *pkt, int riV)
/* fIMMEXT(riV); (riV = riV & ~3); (PC = fREAD_PC()+riV);} */
{
int64_t qemu_tmp_0 = ~((int64_t)3ULL);
int32_t qemu_tmp_1 = riV & qemu_tmp_0;
riV = qemu_tmp_1;
TCGv_i32 tmp_0 = tcg_temp_local_new_i32();
tcg_gen_movi_i32(tmp_0, ctx->base.pc_next);
TCGv_i32 tmp_1 = tcg_temp_local_new_i32();
tcg_gen_addi_i32(tmp_1, tmp_0, (int64_t)riV);
tcg_temp_free_i32(tmp_0);
gen_write_new_pc(tmp_1);
tcg_temp_free_i32(tmp_1);
}

The `(int64_t)riV` cast is actually useless so I simply dropped it, thanks
for pointing it out.

This is all gonna be in the next patchset ofc.

~Paolo

[-- Attachment #2: Type: text/html, Size: 2920 bytes --]