RE: [PATCH v8 29/35] Hexagon (target/hexagon) translation
> -Original Message- > From: Richard Henderson > Sent: Monday, March 15, 2021 8:32 AM > To: Taylor Simpson ; qemu-devel@nongnu.org > Cc: phi...@redhat.com; alex.ben...@linaro.org; laur...@vivier.eu; > a...@rev.ng; Brian Cain > Subject: Re: [PATCH v8 29/35] Hexagon (target/hexagon) translation > > On 3/14/21 9:06 PM, Taylor Simpson wrote: > >> Yes, but DISAS_NORETURN still means we've already exited. > >> > >> Just like calling abort() in C means that we won't reach any following > >> return statement. > > > > Then I'm missing something because the code emitted here does get executed. > > You really are missing the point. > > The code emitted here, for the NORETURN case, gets executed? How do you know? I can see the side effects. For example, there is a call to gen_exec_counters(ctx), and I can see the counters being updated. > And if so, then *something* is returning when it shouldn't. > > The stop hook is for the use of all of the *other* DISAS_* codes, for which > we have not yet exited. > > There should be *nothing* to be done for NORETURN. We have longjmp'ed > away to the main loop already. Anything that needed to be done must have been > done before that point. OK - I'll make sure everything is done during the packet generation and nothing is done during tb_stop. Thanks, Taylor
Re: [PATCH v8 29/35] Hexagon (target/hexagon) translation
On 3/14/21 9:06 PM, Taylor Simpson wrote: Yes, but DISAS_NORETURN still means we've already exited. Just like calling abort() in C means that we won't reach any following return statement. Then I'm missing something because the code emitted here does get executed. You really are missing the point. The code emitted here, for the NORETURN case, gets executed? How do you know? And if so, then *something* is returning when it shouldn't. The stop hook is for the use of all of the *other* DISAS_* codes, for which we have not yet exited. There should be *nothing* to be done for NORETURN. We have longjmp'ed away to the main loop already. Anything that needed to be done must have been done before that point. r~
RE: [PATCH v8 29/35] Hexagon (target/hexagon) translation
> -Original Message-
> From: Richard Henderson
> Sent: Saturday, March 13, 2021 7:44 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: phi...@redhat.com; alex.ben...@linaro.org; laur...@vivier.eu;
> a...@rev.ng; Brian Cain
> Subject: Re: [PATCH v8 29/35] Hexagon (target/hexagon) translation
>
> >> -Original Message-
> >> From: Richard Henderson
> >> Sent: Sunday, February 14, 2021 7:04 PM
> >> To: Taylor Simpson ; qemu-devel@nongnu.org
> >> Cc: phi...@redhat.com; alex.ben...@linaro.org; laur...@vivier.eu;
> >> a...@rev.ng; Brian Cain
> >> Subject: Re: [PATCH v8 29/35] Hexagon (target/hexagon) translation
> >>
> >> On 2/7/21 9:46 PM, Taylor Simpson wrote:
> >>> +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?
>
> Yes, but DISAS_NORETURN still means we've already exited.
>
> Just like calling abort() in C means that we won't reach any following return
> statement.
Then I'm missing something because the code emitted here does get executed. I
thought the tb_stop function is a place for the target to add code. Should I
push this up to all the places where we set ctx->base.is_jmp to DISAS_NORETURN?
Taylor
Re: [PATCH v8 29/35] Hexagon (target/hexagon) translation
On 3/13/21 6:40 PM, Taylor Simpson wrote:
-Original Message-
From: Richard Henderson
Sent: Sunday, February 14, 2021 7:04 PM
To: Taylor Simpson ; qemu-devel@nongnu.org
Cc: phi...@redhat.com; alex.ben...@linaro.org; laur...@vivier.eu;
a...@rev.ng; Brian Cain
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?
Yes, but DISAS_NORETURN still means we've already exited.
Just like calling abort() in C means that we won't reach any following return
statement.
r~
RE: [PATCH v8 29/35] Hexagon (target/hexagon) translation
> -Original Message-
> From: Richard Henderson
> Sent: Sunday, February 14, 2021 7:04 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: phi...@redhat.com; alex.ben...@linaro.org; laur...@vivier.eu;
> a...@rev.ng; Brian Cain
> 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
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.
> +static int read_packet_words(CPUHexagonState *env, DisasContext *ctx,
> + uint32_t words[])
> +{
> +bool found_end = false;
> +int nwords, max_words;
> +
> +memset(words, 0, PACKET_WORDS_MAX * sizeof(uint32_t));
> +for (nwords = 0; !found_end && nwords < PACKET_WORDS_MAX; nwords++) {
> +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.)
> +#if HEX_DEBUG
> +static inline void gen_check_store_width(DisasContext *ctx, int slot_num)
> +{
> +TCGv slot = tcg_const_tl(slot_num);
> +TCGv check = tcg_const_tl(ctx->store_width[slot_num]);
> +gen_helper_debug_check_store_width(cpu_env, slot, check);
> +tcg_temp_free(slot);
> +tcg_temp_free(check);
> +}
> +#define HEX_DEBUG_GEN_CHECK_STORE_WIDTH(ctx, slot_num) \
> +gen_check_store_width(ctx, slot_num)
> +#else
> +#define HEX_DEBUG_GEN_CHECK_STORE_WIDTH(ctx, slot_num) /* nothing */
> +#endif
Fold this to a simple function call:
static void gen_check_store_width(...)
{
if (HEX_DEBUG) {
}
}
> +#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.
> +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.
r~
