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

[Paolo Montesel]

On Wed, Apr 28, 2021 at 5:55 PM Taylor Simpson <tsimpson@quicinc.com> wrote:
>
>
>
> >From: Paolo Montesel <paolo.montesel.revng@gmail.com>
> >Sent: Wednesday, April 28, 2021 5:25 AM
> >To: Taylor Simpson <tsimpson@quicinc.com>
> >Cc: Alessandro Di Federico <ale.qemu@rev.ng>; qemu-devel@nongnu.org; Brian 
> >Cain <bcain@quicinc.com>; nizzo@rev.ng; >philmd@redhat.com; 
> >richard.henderson@linaro.org; Alessandro Di Federico <ale@rev.ng>
> >Subject: Re: [PATCH v4 09/12] target/hexagon: import lexer for idef-parser
> >
> >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
>
> This could be further simplified by doing the add in the parser and generating
>     TCGv tmp_1 = tcg_const_tl(ctx->base.pc_next + riV);
> Have you looked at the host code that is generated?  I would expect it to do 
> the constant folding, so the executed code is OK.  However, there's extra 
> time spent building up TCG that could be avoided.

I agree. Since relative jumps happen somewhat often, I went ahead and
added two immediate types (one for the current PC and one for next
PC).
I also noticed that we were using temps for `rvalue_materialize` when,
in fact, we can simply use `tcg_const`.
Overall it should give a nice improvement.

Anyway, this is how the code looks like now:

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;
int32_t qemu_tmp_2 = ctx->base.pc_next + riV;
TCGv_i32 tmp_0 = tcg_const_i32(qemu_tmp_2);
gen_write_new_pc(tmp_0);
tcg_temp_free_i32(tmp_0);
}

That doesn't look too bad (: