[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v8 29/35] Hexagon (target/hexagon) translation
From: |
Taylor Simpson |
Subject: |
RE: [PATCH v8 29/35] Hexagon (target/hexagon) translation |
Date: |
Sun, 14 Mar 2021 00:40:58 +0000 |
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Sunday, February 14, 2021 7:04 PM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: philmd@redhat.com; alex.bennee@linaro.org; laurent@vivier.eu;
> ale@rev.ng; Brian Cain <bcain@quicinc.com>
> Subject: Re: [PATCH v8 29/35] Hexagon (target/hexagon) translation
>
> On 2/7/21 9:46 PM, Taylor Simpson wrote:
> > +static inline void ctx_log_reg_write(DisasContext *ctx, int rnum)
>
> Drop the inline markup throughout.
I can go through the code and remove unnecessary inline's. However, these
particular inline's are needed because this is a header file. If we remove the
inline and the header gets included in a .c file that doesn't use the function,
we get a "defined but not used" error. Also, we need to keep the inline's in
genptr.c to avoid the same error when we switch an instruction between the
fGEN_TCG and helper implementations (and the idef-parser in the future). Also,
there is one function that needs to be inline for performance reasons. I'll
add a comment for that one.
> > + words[nwords] = cpu_ldl_code(env,
> > + ctx->base.pc_next + nwords *
> > sizeof(uint32_t));
>
> translate_ldl, so that a plugin has access to the packet data. (Note that
> pkt_crosses_page is fine, because that's read-ahead, not reads for the
> current
> packet.)
OK
>
> Fold this to a simple function call:
>
> static void gen_check_store_width(...)
> {
> if (HEX_DEBUG) {
> ....
> }
> }
OK
> > +#if HEX_DEBUG
> > + /* When debugging, only put one packet per TB */
> > + ctx->base.is_jmp = DISAS_TOO_MANY;
> > +#endif
>
> Why? You can always add -singlestep to the command-line.
OK
> > + case DISAS_NORETURN:
> > + gen_exec_counters(ctx);
> > + tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], hex_next_PC);
> > + if (ctx->base.singlestep_enabled) {
> > + gen_exception_debug();
> > + } else {
> > + tcg_gen_exit_tb(NULL, 0);
> > + }
>
> DISAS_NORETURN says that we have *already* exited the TB. None of the
> code you
> emit here will be reachable.
Isn't this called before the TB ends? Here's the code in translator.c
/* Emit code to exit the TB, as indicated by db->is_jmp. */
ops->tb_stop(db, cpu);
gen_tb_end(db->tb, db->num_insns - bp_insn);
Thanks,
Taylor
- RE: [PATCH v8 29/35] Hexagon (target/hexagon) translation,
Taylor Simpson <=