> -----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
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~
> -----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))
© 2016 - 2024 Red Hat, Inc.