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

[Richard Henderson <richard.henderson@linaro.org>]

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~