1. Patchew
  2. QEMU
  3. View series

RE: [RFC PATCH v3 00/34] Hexagon patch series

Taylor Simpson posted 34 patches 5 years, 5 months ago
Only 0 patches received!
There is a newer version of this series
RE: [RFC PATCH v3 00/34] Hexagon patch series
Posted by Taylor Simpson 5 years, 5 months ago 
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Sunday, August 30, 2020 5:33 PM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: philmd@redhat.com; laurent@vivier.eu; riku.voipio@iki.fi;
> aleksandar.m.mail@gmail.com; ale@rev.ng
> Subject: Re: [RFC PATCH v3 00/34] Hexagon patch series
>
> I don't mind intermediate updates.  Just keep a list in the cover letter of the
> things that are still on the to-do list, and I'll not focus on those.
>
> We could also talk about what portions of the to-do list are blocker, and what
> can be done via normal development.  Because neither you nor I want to
> carry
> around this huge patch set forever.

OK, here's the list of items.  Let me know if I missed anything.  I'll indicate which ones can be done quickly and which ones would take more time.  I added a column for blocker if you or anyone else has input on that.

PatchItemEffortBlocker
Use qemu softfloat??Yes
Use qemu decodetree.py??
SeveralUse const when appropriatesmall
SeveralRemove anything after g_assert_not_reachedsmall
SeveralFix log_store32/64 add/remove/add in patch seriessmall
SeveralFollow naming guidelines for structs and enumssmall
04Move decls to cpu-param.hsmall
04Remove CONFIG_USER_ONLY ifdef'ssmall
04Remove DEBUG_HEXAGONsmall
04Remove stack pointer modification hack, use qemu mechanismsmall
04Add property x-lldb-compat to control output in logsmall
06Include instruction and raw bytes in disassemblysmall
07Use DEF_HELPER_FLAGSsmall
07, 26Endianness of merge_bytessmall
07Fix overlap testsmall
07Remove HELPER(debug_value)/HELPER(debug_value_i64)small
09Include "qemu/osdep.h" instead of <stdint.h>small
10 (and others)Stick with stdint.h types except in imported filessmall
11Remove description from reg field definitionssmall
13Move regmap.h into decode.csmall
14, 27Use bit mask instead of strings in decodingsmall
14Add comments to decodersmall
16Use qemu/int128.hmedium
17Squash patches dealing with imported filessmall
24Use qemu/bitops.h for instruction attributessmall
24Fix initialization of opcode_short_semanticssmall
24Change if (p == NULL) { g_assert_not_reached(); } to assert(p != NULL)small
25Expand DECL/READ/WRITE/FREE macros into generated codesmall
26Rewrite fINSERT*, fEXTRACT*, f?XTN macrossmall
26Investigate fRND macrosmall
26Change REG = REG to (VOID)REG to suppress compiler warningsmall
27Remove multiple includes of imported/iclass.defsmall
28Move genptr_helpers.h into genptr.csmall
28Remove unneeded tempssmall
28Use tcg_gen_deposit_tl and tcg_gen_extract_tl when dealing with p3_0small
29Size opcode_genptr[] properly and initialize with [TAG] = generate_##TAGsmall
30Don't generate helpers for instructions that are overriddensmall
Don't include "gen_tcg.h" in helper.h
31Use bitmask for ctx->reg_log instead of an arraysmall
31Use tcg_gen_extract_i32 for gen_slot_cancelled_checksmall
31Properly deal with reading instructions across a page boundary and toomedium
many instructions before finding end-of-packet
31Don't set PC at the beginning of every packetmedium
31Don't set slot_cancelled unless neededsmall
31Don't set hex_pred_written unless neededmedium
31Change cancelled variable to not localsmall
31Remove unnecessary tempsmall
31Let tcg_gen_addi_tl check for zerosmall
31Move gen_exec_counters to end of TB instead of every packetmedium
31Move end of TB handling to hexagon_tr_tb_stopsmall
Re: [RFC PATCH v3 00/34] Hexagon patch series
Posted by Richard Henderson 5 years, 5 months ago 
On 8/31/20 10:57 AM, Taylor Simpson wrote:
> OK, here's the list of items.  Let me know if I missed anything.  I'll 
> indicate which ones can be done quickly and which ones would take more time.
> I added a column for blocker if you or anyone else has input on that.
> 
> PatchItemEffortBlocker
> Use qemu softfloat??Yes

Hmm, this table didn't render.  Below, yes/no for blocker column.

> Use qemu decodetree.py??

no

> SeveralUse const when appropriatesmall

yes

> SeveralRemove anything after g_assert_not_reachedsmall

yes

> SeveralFix log_store32/64 add/remove/add in patch seriessmall

yes

> SeveralFollow naming guidelines for structs and enumssmall

yes

> 04Move decls to cpu-param.hsmall

Yes.  The only reason this even compiled is that you don't do system mode yet.  ;-)

> 04Remove CONFIG_USER_ONLY ifdef'ssmall

yes

> 04Remove DEBUG_HEXAGONsmall

yes

> 04Remove stack pointer modification hack, use qemu mechanismsmall

yes

> 04Add property x-lldb-compat to control output in logsmall

yes

> 06Include instruction and raw bytes in disassemblysmall

yes

> 07Use DEF_HELPER_FLAGSsmall

no, but should be easy.

> 07, 26Endianness of merge_bytessmall

Yes, one way or another; hopefully by removing all of the merge_bytes and using
probe_read.

> 07Fix overlap testsmall

yes

> 07Remove HELPER(debug_value)/HELPER(debug_value_i64)small

yes

> 09Include "qemu/osdep.h" instead of <stdint.h>small

yes

> 10 (and others)Stick with stdint.h types except in imported filessmall

yes

> 11Remove description from reg field definitionssmall

yes

> 13Move regmap.h into decode.csmall

yes

> 14, 27Use bit mask instead of strings in decodingsmall

no, but should be easy.

> 14Add comments to decodersmall

yes

> 16Use qemu/int128.hmedium

no

> 17Squash patches dealing with imported filessmall

yes

> 24Use qemu/bitops.h for instruction attributessmall

no

> 24Fix initialization of opcode_short_semanticssmall

yes

> 24Change if (p == NULL) { g_assert_not_reached(); } to assert(p != NULL)small

no

> 25Expand DECL/READ/WRITE/FREE macros into generated codesmall

Yes.

In the end I think some of these will in the end want to be helper functions.
As I was thinking how to best write A2_add, I was thinking

/* TODO: You currently have an "offset" parameter to
   DECL_REG.  I can't see that it is ever used?
   I would *hope* that this could be resolved earlier,
   so that by this time insn->regno[*] is absolute.  */
static int resolve_regno(Insn *insn, int slot, int off);

/* Return hex_new_value[regno]; log the write. */
static TCGv reg_for_write(DisasContext *ctx, int regno);

/* Return hex_reg[regno]; force up-to-date value for PC. */
static TCGv reg_for_read(DisasContext *ctx, int regno);

/* if (preg) hex_new_value[regno] = hex_reg[regno],
   or !preg if !test_positive.
   Leaves hex_new_value[] canonical for gen_reg_writes,
   no extra temporary required. */
static void gen_cancel_reg(DisasContext *ctx, int preg,
                           int rreg, bool test_positive);

DEF_TCG_FUNC(A2_add)
{
    int rd = resolve_regno(insn, 0, 0);
    int rs = resolve_regno(insn, 1, 0);
    int rt = resolve_regno(insn, 2, 0);

    tcg_gen_add_tl(reg_for_write(ctx, rd),
                   reg_for_read(ctx, rs),
                   reg_for_read(ctx, rt));
}

DEF_TCG_FUNC(A2_paddit)
{
    int pu = resolve_regno(insn, 0, 0);
    int rd = resolve_regno(insn, 1, 0);
    int rs = resolve_regno(insn, 2, 0);
    int rt = resolve_regno(insn, 3, 0);

    tcg_gen_add_tl(reg_for_write(ctx, rd),
                   reg_for_read(ctx, rs),
                   reg_for_read(ctx, rt));
    gen_cancel_reg(ctx, insn, rd, pu, true);
}

However, I don't think we have to have a comprehensive set of these now.  We
can expand everything into the generator to start, then adjust the generator as
we add helper functions and use them within the overrides.

> 26Rewrite fINSERT*, fEXTRACT*, f?XTN macrossmall

yes

> 26Investigate fRND macrosmall

no, but should be easy.

> 26Change REG = REG to (VOID)REG to suppress compiler warningsmall

yes

> 27Remove multiple includes of imported/iclass.defsmall

yes

> 28Move genptr_helpers.h into genptr.csmall

yes

> 28Remove unneeded tempssmall

no

> 28Use tcg_gen_deposit_tl and tcg_gen_extract_tl when dealing with p3_0small

no

> 29Size opcode_genptr[] properly and initialize with [TAG] = generate_##TAGsmall

yes; i think this will fall out of other changes.

> 30Don't generate helpers for instructions that are overriddensmall

yes

> Don't include "gen_tcg.h" in helper.h

yes

> 31Use bitmask for ctx->reg_log instead of an arraysmall

yes

> 31Use tcg_gen_extract_i32 for gen_slot_cancelled_checksmall

yes

> 31Properly deal with reading instructions across a page boundary and toomedium
> many instructions before finding end-of-packet

Yes, this should be easy.  Unless there's some surprise in the code I have
already suggested code.

> 31Don't set PC at the beginning of every packetmedium

no

> 31Don't set slot_cancelled unless neededsmall

no

> 31Don't set hex_pred_written unless neededmedium

no

> 31Change cancelled variable to not localsmall

yes

> 31Remove unnecessary tempsmall

yes

> 31Let tcg_gen_addi_tl check for zerosmall

yes

> 31Move gen_exec_counters to end of TB instead of every packetmedium

no

> 31Move end of TB handling to hexagon_tr_tb_stopsmall

yes


r~
RE: [RFC PATCH v3 00/34] Hexagon patch series
Posted by Taylor Simpson 5 years, 5 months ago 
> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Monday, August 31, 2020 2:44 PM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: philmd@redhat.com; laurent@vivier.eu; riku.voipio@iki.fi;
> aleksandar.m.mail@gmail.com; ale@rev.ng
> Subject: Re: [RFC PATCH v3 00/34] Hexagon patch series
>
> On 8/31/20 10:57 AM, Taylor Simpson wrote:
> > OK, here's the list of items.  Let me know if I missed anything.  I'll
> > indicate which ones can be done quickly and which ones would take more
> time.
> > I added a column for blocker if you or anyone else has input on that.
> >
> > PatchItemEffortBlocker
> > Use qemu softfloat??Yes
>
> Hmm, this table didn't render.  Below, yes/no for blocker column.

Sorry about that - not sure what happened.

I will work all those you marked "yes" or "no, but should be easy".

> > 25Expand DECL/READ/WRITE/FREE macros into generated codesmall
>
> Yes.
>
> In the end I think some of these will in the end want to be helper functions.
> As I was thinking how to best write A2_add, I was thinking

See my response to the thread on patch 30/34.

Since you mention A2_paddit, here's what it would look like assuming it is overridden.

static void generate_A2_paddt(CPUHexagonState *env, DisasContext *ctx, insn_t *insn, packet_t *pkt)
{
/* A2_paddit */
int PuN = insn->regno[0];
TCGv PuV = hex_pred[PuN];
Int RdN = insn->regno[1];
TCGv RdV = tcg_temp_local_new();
if (!is_preloaded(ctx, RdN)) {
    tcg_gen_mov_tl(hex_new_value[RdN], hex_gpr[RdN]);
}
int RsN = insn->regno[2];
TCGv RsV = hex_gpr[RsN];
int siV = insn->immed[0];

fGEN_TCG_A2_paddit({if(fLSBOLD(PuV)){fIMMEXT(siV); RdV=RsV+siV;} else {CANCEL;}});

gen_log_reg_write(RdN, RdV, insn->slot, 1);   /* Only does the write if we haven't cancelled */
ctx_log_reg_write(ctx, RdN);

tcg_temp_free(RdV);
/* A2_paddit */
}

Here's what the override looks like (there are a bunch of these, so we have a helper macro which could also be a function)
/* Predicated add instructions */
#define GEN_TCG_padd(PRED, ADD) \
    do { \
        TCGv LSB = tcg_temp_new(); \
        TCGv mask = tcg_temp_new(); \
        TCGv zero = tcg_const_tl(0); \
        PRED; \
        ADD; \
        tcg_gen_movi_tl(mask, 1 << insn->slot); \
        tcg_gen_or_tl(mask, hex_slot_cancelled, mask); \
        tcg_gen_movcond_tl(TCG_COND_NE, hex_slot_cancelled, LSB, zero, \
                                               hex_slot_cancelled, mask); \
        tcg_temp_free(LSB); \
        tcg_temp_free(mask); \
        tcg_temp_free(zero); \
    } while (0)

#define fGEN_TCG_A2_paddit(SHORTCODE) \
    GEN_TCG_padd(fLSBOLD(PuV), tcg_gen_addi_tl(RdV, RsV, siV))

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.